[hibernate-dev] Re: About HSEARCH-310

thank you, you helped me a lot; still I would like to propose an evolution to cover all use cases correctly. I've already implemented and tested this, in roughly three steps: 1) when moving the cursor outside of the loaded window boundaries I extract the new EntityInfos from the index, then verify if any matching entity is already contained in the persistence context: if it is, I discard the EntityInfo and I "batch load" the entities identified with the remaining EntityInfos ( I am doing this by building the EntityKey to look at session.getPersistenceContext().containsEntity( key ) ).

2) Immediately after batch loading, I evict them all right away and discard the references: this may look useless but the goal was to put data in the second level cache, useful for later and possibly living in a more efficient data structure than a simple soft map.

Please note that I'm evicting only those objects which were not attached before the loading, so nothing changed in the persistence context from a client-code-point-of-view. The original implementation also did a one-by-one load on each entity as a last initialization step; this is not needed here as I use load on each entity later on, when I'm sure the clients actually needs that data. 3) After that when a get() is called I use load( entityInfo ) to get a managed instance to return to the user. The end result is: * I don't put stuff in the persistence context the user doesn't expect

* I don't evict stuff that was attached before

* no memory leaks it works correctly even without a second level cache, but of course you only get good performance when using one.

Do you think this looks reasonable? Besides the EntityKey creation the code is much simplified, and the lines of code are reduced. Would you prefer me to keep a soft reference to the loaded entities to re-attach to the session somehow? I'll prefer to avoid the soft map for the entities: IMHO having an additional (inefficient) caching layer on top of object loading, which has probably lots of other caching layers, is just making the code more complex. Sanne 2009/1/13 Emmanuel Bernard <emmanuel(a)hibernate.org&gt;: > Clearly we cannot solve all use cases, so let's focus on the most > useful > one. > People use scrollable resultset to walk through all objects in a > window one > after the other wo skipping them. > > while( rs.hasNext() ) { > Address a = (Address) rs.get(0)[0]; > doSomething(a); > if (i++ % 100 == 0) s.clear(); > } > > So we should consider that: > - eviction is done by the user > - eviction can happen behind the impl's back > > => > place objects and EntityInfo in a Soft reference (if we need to keep > EntityInfo, don't remember). > when we give an entity, make sure session.contains(entity) == true. > Otherwise, discard the detached one and reload it. > > That should help for most cases. > > > On Jan 11, 2009, at 18:21, Sanne Grinovero wrote: > >> Hello, >> I'm in need of some help about how to fix this memory leak at >> HSEARCH-310 >> in ScrollableResultsImpl of Search, there currently is: >> >> * an internal cache of all loaded EntityInfo >> * an internal cache of all loaded managed Entities >> >> Both caches are never emptied during the scrolling; the >> implementation >> is loading a batch-sized list each time an entity is requested which >> was not loaded before and adds new objects to the caches. >> >> We had said to use some Soft ReferenceMap to fix this, but >> actually we >> should evict the loaded entities which where not requested from the >> client code, or I'll still be leaking memory in Hibernate's >> persistence context. >> >> I expect the most frequent use case for a ScrollableResults should >> be >> to iterate at will and periodically evict objects in the client >> code; >> Still the client could skip some entities we loaded by batching. >> At first I thought to keep track of which references were used and >> evict the other ones, but this isn't a good solution as the client >> could be loading some entities by other means (i.e. another query) >> and >> won't be happy if I'm evicting some random entities: I don't have >> enough information to know for sure what to evict and what is >> going to >> be used later. >> >> Also a bug, consequence of current implementation, is that if you >> get >> an object, then you evict it, and then try scrollable.get() you >> don't >> get an attached object but again the evicted instance; as a client >> of >> the Hibernate library I expect all objects I ask for to be >> attached by >> default. >> >> My idea to fix this should be something like: >> 1) load the batch >> 2) evict them all >> 3) and then load one by one as requested, relying on the second >> level >> cache (I suppose point 1 should have warmed it) >> >> But this has other flaws.. like I should not evict (2) those objects >> already existing in the persistence context. >> Ideally I should avoid to preload objects already present in the >> persistence context; I am considering to look in the >> session.persistenceContext.entitiesByKey map to only load+evict by >> batch those entities not already managed. >> >> Is there some way to populate the second level cache skipping the >> actual load in the persistence context? >> Should I keep my own "soft map cache" in the scroll implementation >> anyway, and somehow detach and reattach them as needed? >> Am I missing some trivial better solution, or is my solution not >> going >> to work at all? >> >> thanks, >> Sanne > >