[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-5927?page=c... ] Lari Hotari commented on HHH-5927: ---------------------------------- HHH-5300 is related. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-5927?page=c... ] Strong Liu reassigned HHH-5927: ------------------------------- Assignee: Strong Liu -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-5927?page=c... ] Strong Liu commented on HHH-5927: --------------------------------- how is the result? -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-5927?page=c... ] Strong Liu commented on HHH-5927: --------------------------------- Lari, have you confirmed this? -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ http://opensource.atlassian.com/projects/hibernate/browse/HHH-5927?page=c... ] Sanne Grinovero commented on HHH-5927: -------------------------------------- I've taken some of the ideas from Strong and created an updated branch at https://github.com/Sanne/hibernate-core/tree/HHH-5927-part it's a set of trivial changes, but I believe it could already reduce contention significantly as every put is currently very expensive, and it was unnecessarily performed for each get. I agree we should have a more sophisticated cache, but since that is quite more complex (more time), I'd be curious to know if just the changes at HHH-5927-part improve your test scenario significantly. I'd say that the proper implementation should be provided by the 2nd level cache provider, if possible. Infinispan definitely provides a modern LIRS implementation which could be used for this, and I expect EHCache to provide something similar: no need to reinvent and maintain another implementation. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Steve Ebersole commented on HHH-5927: ------------------------------------- Couple of things... * @Andres, the branches from Strong and Sanne are based on 4.x, but you could still pull their changes into a 3.6 fork. At least to verify that the changes have desired effect. For example, I think the only real pertinent change on Sanne's in regard to this issue is https://github.com/Sanne/hibernate-orm/commit/4edca8e52bd4df34aadda98202d... and that one should be pretty easy to backport to 3.6 using cherry-pick or diff/patching * Regarding the {{synchonized}} on the {{get}} method, yes we could have gone with a more granular re-entrant read/write lock. That code was added before the switch to Java 5 and at that time we decided against pulling in the oswego library or others just to provide that capability for that one usage. Now that the code bases on Java 6 we could certainly look to change that using a java.util.concurrent.locks.ReentrantReadWriteLock. But I am curious whether {{synchonized}} is the real bottleneck. Hard to say just from the attached profile. * Another option is to get out of writing and maintaining these classes altogether. Its not a core competency of Hibernate as an O/RM solution. Overall this is my preferred solution. There are a couple of options here. *# Using the Google guava library has been suggested. The trouble there is that while they do have "LRU" capability, it is strictly size based; and it is essentially impossible for us to name an appropriate size for all deployments and probably just as impossible expecting users to set that up correctly. See the "semi-roll-our-own" point... *# We could swap in something that is capable of completely replicating the functionality of {{org.hibernate.internal.util.collections.SoftLimitMRUCache}}. Like I mentioned above, guava currently does not fit this bill. Ideally we want that thing to be concurrent (reentrant rather than synchronized) and GC-based. One library that seems to fit all those reqs is http://code.google.com/p/simplelrucache/ I have no experience/affiliation with it. *# We could do a semi-roll-our-own approach where we keep {{org.hibernate.internal.util.collections.SoftLimitMRUCache}} but replace its innards with stuff from guava, http://code.google.com/p/concurrentlinkedhashmap/, etc -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Steve Ebersole commented on HHH-5927: ------------------------------------- # I had thought about frequency-based eviction. Ideally you'd want a policy based on both. # You make a huge assumption that that would "run faster". Caching is always a trade off between processor and memory. You are maybe only considering the processor aspect when saying it will run faster. But if the size we choose along with other stuff going on in that particular JVM cause memory to come close to max the application will most decidely not "run faster"; it will run far slower. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Steve Ebersole commented on HHH-5927: ------------------------------------- And if no caching is enabled? -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Sanne Grinovero commented on HHH-5927: -------------------------------------- One might not want caching for entities, but still benefit from adding a proper cache library. We could of course provide our own implementation as a "simple and easy" one, but one would have to provide a different implementation if special features such as soft or time based evictions are needed. If you want to avoid more jars and extensions, I'd vote to use LIRS only and not have options for different implementations. It seems suited for this case, and we can copy some code from Infinispan, AFAIR it's a pretty self contained implementation. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Eric Dalquist commented on HHH-5927: ------------------------------------ Here is a very fast pass at using Guava's CacheBuilder API to re-write SoftLimitMRUCache. I didn't remove the method-level sync code because I didn't spend enough time reviewing the behavior for thread-saftey but I believe that the sync keyword could be removed and the class will still perform correctly in a non-blocking fashion. https://gist.github.com/1877140 The one thing I didn't fix in this patch is that some tests fail when a null key is used and the Guava Cache APIs don't support null keys or values. That could easily be handled in SoftLimitMRUCache with either short-circuit handling or a placeholder object. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Strong Liu commented on HHH-5927: --------------------------------- oh, it saying the values to be compared using identity -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Eric Dalquist commented on HHH-5927: ------------------------------------ So the problem with MapMaker is you're completely giving up the max-size configuration and relying solely on the GC to cleanup, the big danger there is soft-reference only caches can turn into serious performance problems in low-memory high-demand situations as data is constantly removed from the cache, increasing CPU demands. The dual-map approach addresses this fairly well by keeping more frequently accessed data available no matter what the GC state is. As you noted, the equality check isn't an issue since you're never comparing the values in the cache. I believe in my example you can remove the synchronized keywords as well with the one problem that there could be a cache miss while an entry is being evicted from the strong cache and moved into the weak cache. If we can assume that this case would be rare it wouldn't be a big problem as the plan would be re-generated and placed back into the strong cache, usurping the existing entry. So we would have the very small chance for some extra CPU work with the benefit of being lock-free Also in your gist you set concurrency to 1, are you really only expecting 1 thread to update the table at a time? -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Steve Ebersole commented on HHH-5927: ------------------------------------- QueryPlanCache is part of SessionFactory, not Session. QueryPlanCache absolutely must be thread safe. And it ideally would be reentrant, rather than synchronized -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Steve Ebersole commented on HHH-5927: ------------------------------------- Dunno, did not look at your patch specifically. Just going off of the discussion here. So apparently your patch to this "QueryPlanCache performance" issue addressed more than just QueryPlanCache... -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Sanne Grinovero commented on HHH-5927: -------------------------------------- Is there a unit test able to stress this functionality directly? We need to run a competition across implementations ;) Or at least verify how any new implementation would behave. Strong I like the stile of your patch, I think it's going in the good direction but I'm still of the idea that LIRS would be far superior as eviction strategy and since we could implement [relatively] easily that would avoid the new dependency. Also I'm not a big fan of the soft references strategy, they might make it easier to implement but in practice you'll see memory usage grow and grow until the whole cache is trashed, and potentially the cache of other frameworks doing similar weird things as well. LIRS would need a single configuration parameter: an amount of entries it should attempt to keep (but it's not an exact amount, just an estimate). http://www.cse.ohio-state.edu/hpcs/WWW/HTML/publications/papers/TR-02-6.pdf -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Sanne Grinovero commented on HHH-5927: -------------------------------------- ahh ok, I was mostly concerned about needing to introduce guava just for this use case. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Strong Liu commented on HHH-5927: --------------------------------- https://github.com/hibernate/hibernate-orm/pull/283 1. remove commons-collections dependency. 2. moved org.hibernate.internal.util.collections.ConcurrentReferenceHashMap 3. Using ConcurrentReferenceHashMap this to replace old ReferenceMap (from c 4. copied org.hibernate.internal.util.collections.BoundedConcurrentHashMap f 5. deprecate properties QUERY_PLAN_CACHE_MAX_STRONG_REFERENCES and QUERY_PLA 6. add two new properties QUERY_PLAN_CACHE_PARAMETER_METADATA_MAX_SIZE and Q 7. remove LRUMap, SimpleMRUCache and SoftLimitMRUCache -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://hibernate.onjira.com/browse/HHH-5927?page=com.atlassian.jira.plug... ] Strong Liu resolved HHH-5927. ----------------------------- Resolution: Fixed Fix Version/s: 4.1.1 -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira