<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Manik,</div><div><br></div><div>Thank you for taking a second look at CacheStore.loadAll().</div><div><br></div><div>The possibility of loading the whole cache in memory simply by calling one method is something I cannot live with ;-)</div><div>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.</div><div>Actually, the content of this method is commented out.. (In my local version of the S3 cache store).</div><div><br></div><div>But let's take a closer look at most of loadAll() calls :</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; ">1)LeaveTask.performRehash()</div></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; ">for (InternalCacheEntry ice : cs.loadAll()) if (!statemap.containsKey(ice.getKey())) statemap.addState(ice);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; ">2)CacheLoaderManagerImpl.preload()</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; ">state = loader.loadAll();</div></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; ">for (InternalCacheEntry e : state)</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "> cache.getAdvancedCache().withFlags(SKIP_CACHE_STATUS_CHECK).put(e.getKey(), e.getValue(), e.getLifespan(), MILLISECONDS, e.getMaxIdle(), MILLISECONDS);</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; ">3)RehashControlCommand.pullState()</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "> for (InternalCacheEntry ice : cacheStore.loadAll()) {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "> Object k = ice.getKey();</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "> if (!state.containsKey(k) && shouldAddToMap(k, oldCH, numCopies, self))</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "> state.put(k, ice.toInternalCacheValue());</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "> }</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "> }</div></div></div></div></div><div><br></div><div>Each time loadAll() is used, it is within a 'for' loop.</div><div>I think that having loadAll returning an Iterable<InternalCacheEntry> will solve memory issues.</div><div><br></div><div>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.</div><div>For 2), there should indeed be a limit to the cache eviction "max size" (+1 for the JIRA issue - can I vote ?).</div><div>For 3), a solution like 1) could be applied (calling getKey on each entry after loading *all* the set... sound a little bit expensive ).</div><div><br></div><div>And in the cache store itself (in S3 and File cache stores)... something funny (someone explain the goal of this ?) : </div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "><span class="Apple-tab-span" style="white-space:pre"> </span>protected void purgeInternal() throws CacheLoaderException {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "><span class="Apple-tab-span" style="white-space:pre"> </span>loadAll();</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Monaco; "><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br></div><div>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 ?</div><div><br></div><div>WDYT ? </div><div><br></div><div>BTW, when you say that rehashing only happens when the cache is not shared, could you please confirm that it is related to the <span class="Apple-style-span" style="color: rgb(163, 0, 148); font-family: Monaco; font-size: 11px; ">shared<span style="color: #dbcaba">=</span><span style="color: #5000ff">"true" <span class="Apple-style-span" style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: medium; ">setting available in the xml configuration ?</span></span></span></div><div><br></div><div>Finally, can I be sure that there will never be a rehash of the keys in distributed mode ?</div><div><br></div><div>Cheers,</div><div><br></div><div>Philippe</div><br><div><div>Le 10 déc. 2009 à 18:10, Manik Surtani a écrit :</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Adrian,<br><br>Thanks for the transcript between yourself and Philippe below. Here are my thoughts:<br><br>* loadAll() is generally overused and this can get expensive. I've changed purgeExpired() in certain impls to not use loadAll().<br>* preloading the cache also calls loadAll(). I have a suggestion for this here - <a href="https://jira.jboss.org/jira/browse/ISPN-310">https://jira.jboss.org/jira/browse/ISPN-310</a> - but this won't be in place till 4.1.0.<br>* 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.<br><br>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().<br><br>I have captured this in <br><br><span class="Apple-tab-span" style="white-space:pre"> </span><a href="https://jira.jboss.org/jira/browse/ISPN-311">https://jira.jboss.org/jira/browse/ISPN-311</a><br><br>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.<br><br>Cheers<br>Manik<br><br><br>On 3 Dec 2009, at 16:20, Adrian Cole wrote:<br><br><blockquote type="cite"><adriancole> aloha all<br></blockquote><blockquote type="cite"><pvdyck> hi all<br></blockquote><blockquote type="cite"><adriancole> we are talking about the rehash concern wrt bucket-based<br></blockquote><blockquote type="cite">cachestores<br></blockquote><blockquote type="cite"><pvdyck> here is transcript<br></blockquote><blockquote type="cite"><pvdyck> it seems that it first loops on the set from the store to<br></blockquote><blockquote type="cite">compare the keys with the keys in memory<br></blockquote><blockquote type="cite"><pvdyck> [17:01] pvdyck: the set of keys present in memory will<br></blockquote><blockquote type="cite">always be smaller ... so maybe looping on this one and comparing with<br></blockquote><blockquote type="cite">the keys present in the store is a good optimization<br></blockquote><blockquote type="cite"><pvdyck> [17:01] pvdyck: I will give you the exact file and line in a moment<br></blockquote><blockquote type="cite"><pvdyck> [17:02] pvdyck: ok LeaveTask:74<br></blockquote><blockquote type="cite"><pvdyck> [17:03] pvdyck: actually<br></blockquote><blockquote type="cite">org.infinispan.distribution.LeaveTask line 74 from CR2<br></blockquote><blockquote type="cite"><adriancole> for context, the current design implies a big load, pvdyck, right?<br></blockquote><blockquote type="cite"><pvdyck> (is it sooo early ... is there and #infinispan irc channel<br></blockquote><blockquote type="cite">display next to the coffee machine ? ;-)<br></blockquote><blockquote type="cite"><adriancole> :)<br></blockquote><blockquote type="cite"><adriancole> pvdyk, I can see that changing the loop will reduce the<br></blockquote><blockquote type="cite">possiblity for overloading a node<br></blockquote><blockquote type="cite"><pvdyck> the design implies calling loadAllLockSafe() ... loading all<br></blockquote><blockquote type="cite">the entries (K+V) from the cache -> very bad idea actually<br></blockquote><blockquote type="cite"><adriancole> seems that keys should be in a separate place<br></blockquote><blockquote type="cite"><adriancole> wdyt?<br></blockquote><blockquote type="cite"><adriancole> a lot of large systems have a separate area for metadata<br></blockquote><blockquote type="cite">and payload<br></blockquote><blockquote type="cite"><adriancole> one popular one is git ;)<br></blockquote><blockquote type="cite"><pvdyck> the simple idea of having this loadAll thing is a problem<br></blockquote><blockquote type="cite"><pvdyck> if it ever get called ... I am quite sure the system will hang<br></blockquote><blockquote type="cite"><pvdyck> and indeed you are right, there is no reason to bring the<br></blockquote><blockquote type="cite">values with it ... keys are more than enough!<br></blockquote><blockquote type="cite"><adriancole> so, here's the thing<br></blockquote><blockquote type="cite"><adriancole> the whole bucket-based thing is suppopsed to help avoid<br></blockquote><blockquote type="cite">killing entries who share the same hashCode<br></blockquote><blockquote type="cite"><adriancole> and there's also another issue with encoding keys<br></blockquote><blockquote type="cite"><adriancole> since they might be typed and not strings<br></blockquote><blockquote type="cite"><pvdyck> is it still the case with the permanent hash ?<br></blockquote><blockquote type="cite"><pvdyck> oops sorry ... consistent hash<br></blockquote><blockquote type="cite"><adriancole> well, I'm talking about the hash of the thing you are putting in<br></blockquote><blockquote type="cite"><adriancole> not the consistent hash alg<br></blockquote><blockquote type="cite"><pvdyck> ok, understood...<br></blockquote><blockquote type="cite"><adriancole> ya, so I think that if we address this, we're ok<br></blockquote><blockquote type="cite"><adriancole> in the blobstore (jclouds) thing, we could address<br></blockquote><blockquote type="cite"><adriancole> by having a partition on hashCode<br></blockquote><blockquote type="cite"><adriancole> and encoding the object's key as the name of the thing in s3<br></blockquote><blockquote type="cite"><adriancole> or whereever<br></blockquote><blockquote type="cite"><adriancole> so like "apple" -> "bear"<br></blockquote><blockquote type="cite"><adriancole> "bear".hashCode/encode("apple")<br></blockquote><blockquote type="cite"><pvdyck> actually, I don't think the problem should end up in the<br></blockquote><blockquote type="cite">hands of the store itself<br></blockquote><blockquote type="cite"><adriancole> that would be convenient :)<br></blockquote><blockquote type="cite"><adriancole> in that case, I think that ispn may need a metadata<br></blockquote><blockquote type="cite">store and value store<br></blockquote><blockquote type="cite"><adriancole> since currently the typed bucket object contains both<br></blockquote><blockquote type="cite">keys and values<br></blockquote><blockquote type="cite"><adriancole> which makes it impossible to get to one without the other<br></blockquote><blockquote type="cite"><adriancole> I'm pretty sure<br></blockquote><blockquote type="cite"><pvdyck> looks like a lot of changes... but it is a path to explore!<br></blockquote><blockquote type="cite"><pvdyck> we obviously need to wait for them to appear ;-)<br></blockquote><blockquote type="cite"><adriancole> firstly, I think we'll need a patch for the s3<br></blockquote><blockquote type="cite">optimization so you can work :)<br></blockquote><blockquote type="cite"><adriancole> and also, this design needs to be reworked for sure<br></blockquote><blockquote type="cite">* Received a malformed DCC request from pvdyck.<br></blockquote><br>--<br>Manik Surtani<br>manik@jboss.org<br>Lead, Infinispan<br>Lead, JBoss Cache<br>http://www.infinispan.org<br>http://www.jbosscache.org<br><br><br><br><br><br>_______________________________________________<br>infinispan-dev mailing list<br>infinispan-dev@lists.jboss.org<br>https://lists.jboss.org/mailman/listinfo/infinispan-dev<br></div></blockquote></div><br></body></html>