On 2/8/15 5:55 PM, sebb wrote: > On 8 February 2015 at 16:57, Phil Steitz <[email protected]> wrote: >> On 2/8/15 8:51 AM, sebb wrote: >>> On 6 February 2015 at 22:36, Phil Steitz <[email protected]> wrote: >>>> On 2/6/15 1:28 PM, sebb wrote: >>>>> On 6 February 2015 at 19:58, Phil Steitz <[email protected]> wrote: >>>>>> On 2/6/15 11:56 AM, sebb wrote: >>>>>>> There seem to be a few use-cases for pools that always treat different >>>>>>> instances as different entries, rather than using the current equals() >>>>>>> check. >>>>>> Yes >>>>>>> Would it make sense to implement alternative versions that accept an >>>>>>> object and wrap it in a class that implements equals() using == and >>>>>>> System.identityHashcode? >>>>>> Yes, we should definitely support this use case. >>>>>>> The IdentityWrapper class was suggested here: >>>>>>> >>>>>>> https://issues.apache.org/jira/browse/POOL-283?focusedCommentId=14307637&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14307637 >>>>>>> >>>>>>> I think it could be done by subclassing the existing implementations >>>>>>> to provide the wrapping/unwrapping support. >>>>>>> >>>>>>> There would be an overhead because the wrappers would need to be >>>>>>> created every time an object was passed in. The wrappers are very >>>>>>> small. >>>>>> Once I get the DBCP release I am working on out, I plan to explore >>>>>> all three alternatives in my comment on POOL-284. The performance >>>>>> regression testing tool that Thomas found and referenced on that >>>>>> ticket looks very interesting and could help assessing the cost of >>>>>> wrapping / unwrapping or changing the current impl to use >>>>>> sync-protected IdentityHashMap. >>>>> Changing to IdentityHashMap implies dropping support for >>>>> equals()-equivalent pool entries. >>>>> Are there no use-cases for such behaviour? >>>> Good point. We would probably want to make this configurable. >>> We seem to be moving towards changing the implementations to support >>> both the current equals() comparison and a new identity comparison >>> (whether by wrapper or alternate hashmap implementation). >> I agree we need to provide something that works for non-HashMap >> compatible objects. >> >> I am not ready to conclude though that we should change the main / >> default impl until we have done some performance testing, which is >> very tricky to get right. I haven't played with the tool that >> Thomas referenced; but it looks promising for this kind of thing. >> In any case, I don't want to do anything that impacts performance >> of DBCP and other clients that work fine under the Hashmap compat >> requirement. > I'm not suggesting changing the default implementation behaviour; that > might break some existing apps. > >>> That would allow Pool2 to be used in the same way as Pool1 (with >>> suitable config). >>> I think that would be better than providing the identity comparing >>> pools as independent PoolUtils methods. >> The advantage of the separate impl approach is we can keep the >> current impl simple and performant. > Perhaps, but I imagine a lot of the code would have to be duplicated. > > I was thinking instead of changing the code so it uses an abstract Map. > When the instance is created, the map type would be chosen > accordingly, with the default as now.
Good idea. Sorry, I was not clear. I don't want to duplicate lots of impl code. The abstract Map idea might work. I just don't want to shove wrapping/unwrapping / extra testing / ideally anything into the default impl. > >>> Also it should make it easier to change the identity implementation at >>> a later date if necessary. >>> I.e. the initial impl. could use the IdentityWrapper, as that is known >>> to work and is available now. >>> If a bettter impl. were developed later it could replace the >>> IdentityWrapper. >> As an alternative impl, I have no problem with this; but I don't >> want to add this overhead to the default unless it really can be >> shown to be costless (which I doubt). > The problem with an alternative impl. is the amount of code that has > to be duplicated. > >> It could be we can just do the "alternative impl" stuff behind the >> scenes in config. The main points IMO are >> >> 0) we don't want to do anything to make current performance worse >> 1) we need to make the contract clear > Agreed. > >> 2) we should provide support for reference-based instance management >> (both the use case where equals does not discern distinct objects >> and where mutability causes equals to "incorrectly" discern >> returning instances from references to them in allObjects). > Not quite sure what you mean by reference-based here. > Is that different from the IdentityWrapper approach? I was just trying to state the core requirement of the default implementation - that from the pool's perspective, identity is equality of reference. The IdentityWrapper is one way to satisfy this requirement. Phil > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
