<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On 10 Dec 2009, at 18:13, philippe van dyck wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div 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></blockquote><div><br></div><div>loadAll() returns a Set<InternalCacheEntry>, which extends Iterable<InternalCacheEntry> - and the only method used on the set is iterator(), so for all practical purposes it does return an Iterable<InternalCacheEntry>. Perhaps you want to restrict the return type so all you can do is iterate, and thus be able to spool entries off the CacheStore lazily, maybe with a prefetch? This makes sense, and will solve memory issues in the CacheStore but not elsewhere (see below).</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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></blockquote><div><br></div><div>That doesn't help. What the loop does is identifies which entries in the cache store are *not* in the state map, and add these to the state map.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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></blockquote><div><br></div><div>Yes, please do vote on the JIRA - even contribute a fix if you feel like, it shouldn't be too hard. :-)</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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></blockquote><div><br></div><div>Yes, (3) and (1) are very similar, but in the same way your approach doesn't help here. Also, entry.getKey() is very cheap, this is not the expensive bit. Loading all of the entries into memory is.</div><div><br></div><div>As a solution for both 1 and 3 is to come up with a stream-like API for loading entries. It could be something low-level like loadAllStream(), but that would mean exposing internal storage formats outside of the CacheStore impl which is bad. The other approach as you suggested, is to return an Iterable (or better still, an Iterator), which lazily loads and pre-fetches. This could be interesting, since entries could be spooled out of the CacheStore but for it to be of any real benefit, the rehashing needs to stream state to the recipient as well otherwise the statemap becomes the memory hog (loops in 1 and 3 iterator over the set and adds relevant entries to the statemap for sending via RPC). ISPN-284 modifies rehashing to stream state to recipients rather than use RPC, and once we have this then spooling entries off the CacheStore will be of benefit. See <a href="https://jira.jboss.org/jira/browse/ISPN-284">https://jira.jboss.org/jira/browse/ISPN-284</a> for more details. Again, you are welcome to lend a hand here too. :-)</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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></blockquote><div><br></div><div>Yes, this was poor code and probably a cheap shortcut, see the updated code in trunk (or CR3) for a better version</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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="font-family: Helvetica; font-size: medium; ">setting available in the xml configuration ?</span></span></span></div></div></blockquote><div><br></div><div>Yes. </div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Finally, can I be sure that there will never be a rehash of the keys in distributed mode ?</div></div></blockquote><div><br></div><div>No - of course there could be a rehash. If you don't rehash, the consistent hash algorithm breaks down when the shape of a cluster changes (nodes added/removed).</div><div><br></div><div><div>Cheers</div><div>Manik</div><div><br></div></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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><a href="mailto:manik@jboss.org">manik@jboss.org</a><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></div>_______________________________________________<br>infinispan-dev mailing list<br><a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>https://lists.jboss.org/mailman/listinfo/infinispan-dev</blockquote></div><br><div>
<span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>--</div><div>Manik Surtani</div><div><a href="mailto:manik@jboss.org">manik@jboss.org</a></div><div>Lead, Infinispan</div><div>Lead, JBoss Cache</div><div><a href="http://www.infinispan.org">http://www.infinispan.org</a></div><div><a href="http://www.jbosscache.org">http://www.jbosscache.org</a></div><div><br></div></div></span><br class="Apple-interchange-newline"></span><br class="Apple-interchange-newline">
</div>
<br></body></html>