On 8 February 2015 at 16:57, Phil Steitz <phil.ste...@gmail.com> wrote: > On 2/8/15 8:51 AM, sebb wrote: >> On 6 February 2015 at 22:36, Phil Steitz <phil.ste...@gmail.com> wrote: >>> On 2/6/15 1:28 PM, sebb wrote: >>>> On 6 February 2015 at 19:58, Phil Steitz <phil.ste...@gmail.com> 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. >> >> 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? --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
