Community
Participate
Working Groups
Build Identifier: While profiling our workload of loading lots of CDOObjects from a CDOResource, we noticed a performance issue in CDORevisionCacheImpl.getObjectType(CDOID), where linear searching happens on an increasingly large collection. The proposed patch utilises a separate HashMap<CDOID, EClass> to speed up these lookups. Reproducible: Always
Created attachment 189156 [details] Proposed patch for the performance issue.
Thank you for your contribution. It looks good and I'd be happy to commit it to the 4.0 stream. Please confirm that: 1) The number of lines that you changed is smaller than 250. 2) You are the only author of these changed lines. 3) You apply the EPL to these changed lines.
(In reply to comment #2) > It looks good and I'd be happy to commit it to the 4.0 stream. Looking at it again I fear that your solution does not scale well. The new map will grow with the numbers of objects that are loaded and it will never shrink. This seems to be acceptable if the set of objects that are loaded is rather small and does not changed that much over time.
1) The number of lines that you changed is smaller than 250. The patch is 64 lines, so... 2) You are the only author of these changed lines. Yes, I am the only author. 3) You apply the EPL to these changed lines. Yes, I apply the EPL.
(In reply to comment #3) > (In reply to comment #2) > > It looks good and I'd be happy to commit it to the 4.0 stream. > > Looking at it again I fear that your solution does not scale well. The new map > will grow with the numbers of objects that are loaded and it will never shrink. > This seems to be acceptable if the set of objects that are loaded is rather > small and does not changed that much over time. I see your point. I can think of three possible solutions: - add a typeMap.remove(id) to removeRevision() - use an LRU Map with a maximum size (see LinkedHashMap.removeEldestEntry()) - use a WeakHashMap and let the garbage collector manage size.
(In reply to comment #5) > - add a typeMap.remove(id) to removeRevision() That does not sound good. That would remove the cached mapping with each object change. > - use a WeakHashMap and let the garbage collector manage size. That would only work if each logical ID has only ever one CDOID instance associated. Not sure if we can guarantee that. > - use an LRU Map with a maximum size (see LinkedHashMap.removeEldestEntry()) That sounds good ;-)
LRU map is good idea but we will need to answer what boundary to set, how to change it and support implementation (we don't have LRU map currently under CDO). Idea is to bound current <CDOID, EClass> map with already existing cache and its eviction policy. How about to have Map<CDOID, Pair<EClass,Integer>>. After chat with Eike, I think it could be good solution (actually it was Eike idea :) ). We could keep track how many revisions are present for particular CDOID in the cache with "Integer". Tracking could be done in a methods addRevision or removeRevision. I suppose that removeRevision is always called on eviction from the cache but I could be wrong. Whenever "Integer == 0", we clear CDOID from the map.
(In reply to comment #7) > How about to have Map<CDOID, Pair<EClass,Integer>>. In principle okay, but I'd suggest to create a special value class TypeAndRefCounter. > After chat with Eike, I > think it could be good solution (actually it was Eike idea :) ). We could keep > track how many revisions are present for particular CDOID in the cache with > "Integer". Tracking could be done in a methods addRevision or removeRevision. I > suppose that removeRevision is always called on eviction from the cache Yes, I think so.
Created attachment 189880 [details] patch v2 - with type map self decreasing
Created attachment 190019 [details] Patch v3 - ready to be committed There was a bug in TypeAndRefCounter.decrease(). You used counter-- but it must be --counter.
Committed to trunk, revision 7311
Available in R20110608-1407
