Guys I just noticed this in the codebase: http://fisheye.jboss.org/browse/Infinispan/branches/4.2.x/core/src/main/j... This seems to have come in as a part of changeset 2518 - http://fisheye.jboss.org/changelog/Infinispan/?cs=2518 - for ISPN-679 (Cache.values does not work correctly). Please be careful when doing stuff like this - the new method added to AbstractLocalCommand effectively creates a new collection the size of the entire data container and loads it with (almost) all keys. An Infinispan node running close to capacity will almost certainly die with an OOM if you do this. It will even OOM if you are not necessarily close to capacity (under half your heap used) and you have > 1 thread concurrently invoking this method. I presume this is to make Cache.keySet(), Cache.values() and Cache.size() transactionally accurate, but remember that this comes at a huge cost. Which is why we chose to go with a best-effort approach. See Javadocs on Cache [1]. The relevant bit states: "Other methods such as Map.size() provide an approximation-only, and should not be relied on for an accurate picture as to the size of the entire, distributed cache. Remote nodes are not queried and in-fly transactions are not taken into account, even if Map.size() is invoked from within such a transaction." Could we please discuss why we have added this? Cheers Manik [1] http://docs.jboss.org/infinispan/4.2/apidocs/org/infinispan/Cache.html -- Manik Surtani manik(a)jboss.org Lead, Infinispan Lead, JBoss Cache http://www.infinispan.org http://www.jbosscache.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

I spotted it too while investigating the behavior of SizeCommand and EntrySetCommand. Map.size() doesn't need to be accurate, but we can get accurate enough value by just subtracting the number of removed entries in the current transaction. SizeCommand therefore can be optimized. KeySetCommand is a different beast. We need to be accurate in the same transaction: tx.begin(); cache.put("newKey", "newValue"); assert cache.keySet().contains("newKey"); The current KeySetCommand implementation uses getKeySetWithinTransaction() and it creates a new set as you pointed out. We could write a wrapper set to minimize memory consumption. Same applies to EntrySetCommand. However, if someone ever passes the keySet or entrySet to other thread, it will be a disaster. Perhaps we should document it clearly? Manik Surtani wrote: > Guys > > I just noticed this in the codebase: > > http://fisheye.jboss.org/browse/Infinispan/branches/4.2.x/core/src/main/j... > > This seems to have come in as a part of changeset 2518 - http://fisheye.jboss.org/changelog/Infinispan/?cs=2518 - for ISPN-679 (Cache.values does not work correctly). > > Please be careful when doing stuff like this - the new method added to AbstractLocalCommand effectively creates a new collection the size of the entire data container and loads it with (almost) all keys. An Infinispan node running close to capacity will almost certainly die with an OOM if you do this. It will even OOM if you are not necessarily close to capacity (under half your heap used) and you have > 1 thread concurrently invoking this method. > > I presume this is to make Cache.keySet(), Cache.values() and Cache.size() transactionally accurate, but remember that this comes at a huge cost. Which is why we chose to go with a best-effort approach. See Javadocs on Cache [1]. The relevant bit states: > > "Other methods such as Map.size() provide an approximation-only, and should not be relied on for an accurate picture as to the size of the entire, distributed cache. Remote nodes are not queried and in-fly transactions are not taken into account, even if Map.size() is invoked from within such a transaction." > > Could we please discuss why we have added this? > > Cheers > Manik > > [1] http://docs.jboss.org/infinispan/4.2/apidocs/org/infinispan/Cache.html > -- > Manik Surtani > manik(a)jboss.org > Lead, Infinispan > Lead, JBoss Cache > http://www.infinispan.org > http://www.jbosscache.org > > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Trustin Lee - http://gleamynode.net/ _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Delegate if there's no related entry in lookedUpEntries. Otherwise +/- based on the information provided by lookedUpEntries. Filtering keySet is fairly easy, but entrySet is a different story. I'm working on it anyway. Manik Surtani wrote: > +1. About your wrapper for keyset, entryset - are you suggesting that it delegates to the data container, + or - the entries added/removed in the current tx context? > > It would need to be very clearly documented that it cannot be handed off to a separate thread. > > On 21 Oct 2010, at 15:57, 이희승 (Trustin Lee) wrote: > >> I spotted it too while investigating the behavior of SizeCommand and >> EntrySetCommand. >> >> Map.size() doesn't need to be accurate, but we can get accurate enough >> value by just subtracting the number of removed entries in the current >> transaction. SizeCommand therefore can be optimized. >> >> KeySetCommand is a different beast. We need to be accurate in the same >> transaction: >> >> tx.begin(); >> cache.put("newKey", "newValue"); >> assert cache.keySet().contains("newKey"); >> >> The current KeySetCommand implementation uses >> getKeySetWithinTransaction() and it creates a new set as you pointed >> out. We could write a wrapper set to minimize memory consumption. Same >> applies to EntrySetCommand. >> >> However, if someone ever passes the keySet or entrySet to other thread, >> it will be a disaster. Perhaps we should document it clearly? >> >> Manik Surtani wrote: >>> Guys >>> >>> I just noticed this in the codebase: >>> >>> http://fisheye.jboss.org/browse/Infinispan/branches/4.2.x/core/src/main/j... >>> >>> This seems to have come in as a part of changeset 2518 - http://fisheye.jboss.org/changelog/Infinispan/?cs=2518 - for ISPN-679 (Cache.values does not work correctly). >>> >>> Please be careful when doing stuff like this - the new method added to AbstractLocalCommand effectively creates a new collection the size of the entire data container and loads it with (almost) all keys. An Infinispan node running close to capacity will almost certainly die with an OOM if you do this. It will even OOM if you are not necessarily close to capacity (under half your heap used) and you have > 1 thread concurrently invoking this method. >>> >>> I presume this is to make Cache.keySet(), Cache.values() and Cache.size() transactionally accurate, but remember that this comes at a huge cost. Which is why we chose to go with a best-effort approach. See Javadocs on Cache [1]. The relevant bit states: >>> >>> "Other methods such as Map.size() provide an approximation-only, and should not be relied on for an accurate picture as to the size of the entire, distributed cache. Remote nodes are not queried and in-fly transactions are not taken into account, even if Map.size() is invoked from within such a transaction." >>> >>> Could we please discuss why we have added this? >>> >>> Cheers >>> Manik >>> >>> [1] http://docs.jboss.org/infinispan/4.2/apidocs/org/infinispan/Cache.html >>> -- >>> Manik Surtani >>> manik(a)jboss.org >>> Lead, Infinispan >>> Lead, JBoss Cache >>> http://www.infinispan.org >>> http://www.jbosscache.org >>> >>> >>> >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> -- >> Trustin Lee - http://gleamynode.net/ >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > -- > Manik Surtani > manik(a)jboss.org > Lead, Infinispan > Lead, JBoss Cache > http://www.infinispan.org > http://www.jbosscache.org > > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev