[infinispan-dev] Improving CacheStore.loadAll()
philippe van dyck
pvdyck at gmail.com
Thu Dec 10 13:13:26 EST 2009
Manik,
Thank you for taking a second look at CacheStore.loadAll().
The possibility of loading the whole cache in memory simply by calling one method is something I cannot live with ;-)
What I mean is that I will not put this code in production knowing that *it will* kill my JVM with a "Java Heap Space" exception.
Actually, the content of this method is commented out.. (In my local version of the S3 cache store).
But let's take a closer look at most of loadAll() calls :
1)LeaveTask.performRehash()
for (InternalCacheEntry ice : cs.loadAll()) if (!statemap.containsKey(ice.getKey())) statemap.addState(ice);
2)CacheLoaderManagerImpl.preload()
state = loader.loadAll();
for (InternalCacheEntry e : state)
cache.getAdvancedCache().withFlags(SKIP_CACHE_STATUS_CHECK).put(e.getKey(), e.getValue(), e.getLifespan(), MILLISECONDS, e.getMaxIdle(), MILLISECONDS);
3)RehashControlCommand.pullState()
for (InternalCacheEntry ice : cacheStore.loadAll()) {
Object k = ice.getKey();
if (!state.containsKey(k) && shouldAddToMap(k, oldCH, numCopies, self))
state.put(k, ice.toInternalCacheValue());
}
}
Each time loadAll() is used, it is within a 'for' loop.
I think that having loadAll returning an Iterable<InternalCacheEntry> will solve memory issues.
Regarding 1), since the total size of the cache store will IMHO almost always be bigger than the entries in memory it may be a better idea to iterate on statemap and use containsKey() on the cache store.
For 2), there should indeed be a limit to the cache eviction "max size" (+1 for the JIRA issue - can I vote ?).
For 3), a solution like 1) could be applied (calling getKey on each entry after loading *all* the set... sound a little bit expensive ).
And in the cache store itself (in S3 and File cache stores)... something funny (someone explain the goal of this ?) :
protected void purgeInternal() throws CacheLoaderException {
loadAll();
}
Could a minimal change in the CacheStore interface (using Iterable instead of returning the whole Set) and a couple of for loop optimizations solve it all ?
WDYT ?
BTW, when you say that rehashing only happens when the cache is not shared, could you please confirm that it is related to the shared="true" setting available in the xml configuration ?
Finally, can I be sure that there will never be a rehash of the keys in distributed mode ?
Cheers,
Philippe
Le 10 déc. 2009 à 18:10, Manik Surtani a écrit :
> Adrian,
>
> Thanks for the transcript between yourself and Philippe below. Here are my thoughts:
>
> * loadAll() is generally overused and this can get expensive. I've changed purgeExpired() in certain impls to not use loadAll().
> * preloading the cache also calls loadAll(). I have a suggestion for this here - https://jira.jboss.org/jira/browse/ISPN-310 - but this won't be in place till 4.1.0.
> * rehashing isn't as bad as you think - the rehashing of entries in stores only takes place when the cache store is *not* shared. Any use of an expensive, remote store (such as S3, JDBC) would typically be shared between Infinispan nodes and as such these will not be considered when rehashing.
>
> That said, stuff can be improved a bit, specifically with the addition of something like loadKeys(Set<Object> excludes). This will allow the rehash code to load just the necessary keys, excluding keys already considered from the data container directly, and then inspect each key to test if the key needs to be rehashed elsewhere. If so, the value could be loaded using load().
>
> I have captured this in
>
> https://jira.jboss.org/jira/browse/ISPN-311
>
> The problem, I think, with maintaining metadata separately is that it adds an additional synchronization point when updating that metadata, whether this is expiration data per key, or even just a list of keys in the store for a quick loadKeys() impl. But am open to ideas, after all this is just CacheStore specific implementation details.
>
> Cheers
> Manik
>
>
> On 3 Dec 2009, at 16:20, Adrian Cole wrote:
>
>> <adriancole> aloha all
>> <pvdyck> hi all
>> <adriancole> we are talking about the rehash concern wrt bucket-based
>> cachestores
>> <pvdyck> here is transcript
>> <pvdyck> it seems that it first loops on the set from the store to
>> compare the keys with the keys in memory
>> <pvdyck> [17:01] pvdyck: the set of keys present in memory will
>> always be smaller ... so maybe looping on this one and comparing with
>> the keys present in the store is a good optimization
>> <pvdyck> [17:01] pvdyck: I will give you the exact file and line in a moment
>> <pvdyck> [17:02] pvdyck: ok LeaveTask:74
>> <pvdyck> [17:03] pvdyck: actually
>> org.infinispan.distribution.LeaveTask line 74 from CR2
>> <adriancole> for context, the current design implies a big load, pvdyck, right?
>> <pvdyck> (is it sooo early ... is there and #infinispan irc channel
>> display next to the coffee machine ? ;-)
>> <adriancole> :)
>> <adriancole> pvdyk, I can see that changing the loop will reduce the
>> possiblity for overloading a node
>> <pvdyck> the design implies calling loadAllLockSafe() ... loading all
>> the entries (K+V) from the cache -> very bad idea actually
>> <adriancole> seems that keys should be in a separate place
>> <adriancole> wdyt?
>> <adriancole> a lot of large systems have a separate area for metadata
>> and payload
>> <adriancole> one popular one is git ;)
>> <pvdyck> the simple idea of having this loadAll thing is a problem
>> <pvdyck> if it ever get called ... I am quite sure the system will hang
>> <pvdyck> and indeed you are right, there is no reason to bring the
>> values with it ... keys are more than enough!
>> <adriancole> so, here's the thing
>> <adriancole> the whole bucket-based thing is suppopsed to help avoid
>> killing entries who share the same hashCode
>> <adriancole> and there's also another issue with encoding keys
>> <adriancole> since they might be typed and not strings
>> <pvdyck> is it still the case with the permanent hash ?
>> <pvdyck> oops sorry ... consistent hash
>> <adriancole> well, I'm talking about the hash of the thing you are putting in
>> <adriancole> not the consistent hash alg
>> <pvdyck> ok, understood...
>> <adriancole> ya, so I think that if we address this, we're ok
>> <adriancole> in the blobstore (jclouds) thing, we could address
>> <adriancole> by having a partition on hashCode
>> <adriancole> and encoding the object's key as the name of the thing in s3
>> <adriancole> or whereever
>> <adriancole> so like "apple" -> "bear"
>> <adriancole> "bear".hashCode/encode("apple")
>> <pvdyck> actually, I don't think the problem should end up in the
>> hands of the store itself
>> <adriancole> that would be convenient :)
>> <adriancole> in that case, I think that ispn may need a metadata
>> store and value store
>> <adriancole> since currently the typed bucket object contains both
>> keys and values
>> <adriancole> which makes it impossible to get to one without the other
>> <adriancole> I'm pretty sure
>> <pvdyck> looks like a lot of changes... but it is a path to explore!
>> <pvdyck> we obviously need to wait for them to appear ;-)
>> <adriancole> firstly, I think we'll need a patch for the s3
>> optimization so you can work :)
>> <adriancole> and also, this design needs to be reworked for sure
>> * Received a malformed DCC request from pvdyck.
>
> --
> Manik Surtani
> manik at jboss.org
> Lead, Infinispan
> Lead, JBoss Cache
> http://www.infinispan.org
> http://www.jbosscache.org
>
>
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20091210/6d16d4fb/attachment-0002.html
More information about the infinispan-dev
mailing list