Cool! Did not know about this! Is this original idea or others are already doing this? On 12-10-19 8:31 AM, Galder Zamarreño wrote: > Hi all, > > Re: https://github.com/galderz/infinispan/commit/0609207d13216de81d77ff51dc20... > > Please avoid using these JDK methods where possible. I've created alternative versions in InfinispanCollections util class that provide a better implementation. > > The problem with the JDK versions is that even if these collections are immutable, if you wanna iterate them (i.e. for(X : emptySet) ), they'll create a brand new Iterator every time they're looped, and that generates useless garbage. > > The InfinispanCollections versions of emptyX will return a constant singleton iterator which avoids this problem, and avoids the need for client code to check if the collection is empty before looping. > > Cheers, > -- > Galder Zamarreño > galder(a)redhat.com > twitter.com/galderz > > Project Lead, Escalante > http://escalante.io > > Engineer, Infinispan > http://infinispan.org > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 10/19/2012 06:28 PM, Dan Berindei wrote: > Galder, what JDK are you using? OpenJDK 1.7 uses > EmptyIterator.EMPTY_ITERATOR since 2007: > > http://hg.openjdk.java.net/jdk7/jdk7/jdk/annotate/37a05a11f281/src/share/... > > I don't have JDK 1.6 sources on hand to check, but I don't think it's > worth optimizing for such an old version anyway. An old version which is probably in use in most production environments :)

From: infinispan-dev-bounces(a)lists.jboss.org [mailto:infinispan-dev-bounces@lists.jboss.org] On Behalf Of Dan Berindei Sent: Friday, October 19, 2012 3:55 PM To: infinispan -Dev List Subject: Re: [infinispan-dev] Avoid Collections.emptySet() / emptyMap() / emptyList On Fri, Oct 19, 2012 at 7:50 PM, Tristan Tarrant <ttarrant(a)redhat.com&gt; wrote: On 10/19/2012 06:28 PM, Dan Berindei wrote: > Galder, what JDK are you using? OpenJDK 1.7 uses > EmptyIterator.EMPTY_ITERATOR since 2007: > > > http://hg.openjdk.java.net/jdk7/jdk7/jdk/annotate/37a05a11f281/src/share/... > > I don't have JDK 1.6 sources on hand to check, but I don't think it's > worth optimizing for such an old version anyway. An old version which is probably in use in most production environments :) If they aren't ready to upgrade to JDK7, I doubt they're willing to upgrade to Infinispan 5.2 :) FYI, I’m using ISPN 5.2 on JDK6 (on a hacked up JBossESB container). Plus who knows what wacky configurations you’re supporting on the JDG side. _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Galder, what JDK are you using? OpenJDK 1.7 uses EmptyIterator.EMPTY_ITERATOR since 2007: http://hg.openjdk.java.net/jdk7/jdk7/jdk/annotate/37a05a11f281/src/share/... I don't have JDK 1.6 sources on hand to check, but I don't think it's worth optimizing for such an old version anyway. Cheers Dan On Fri, Oct 19, 2012 at 5:50 PM, Vladimir Blagojevic <vblagoje(a)redhat.com <mailto:vblagoje@redhat.com>> wrote: Cool! Did not know about this! Is this original idea or others are already doing this? On 12-10-19 8:31 AM, Galder Zamarreño wrote: > Hi all, > > Re: https://github.com/galderz/infinispan/commit/0609207d13216de81d77ff51dc20... > > Please avoid using these JDK methods where possible. I've created alternative versions in InfinispanCollections util class that provide a better implementation. > > The problem with the JDK versions is that even if these collections are immutable, if you wanna iterate them (i.e. for(X : emptySet) ), they'll create a brand new Iterator every time they're looped, and that generates useless garbage. > > The InfinispanCollections versions of emptyX will return a constant singleton iterator which avoids this problem, and avoids the need for client code to check if the collection is empty before looping. > > Cheers, > -- > Galder Zamarreño > galder(a)redhat.com <mailto:galder@redhat.com> > twitter.com/galderz <http://twitter.com/galderz> > > Project Lead, Escalante > http://escalante.io > > Engineer, Infinispan > http://infinispan.org > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Oct 19, 2012, at 7:20 PM, David M. Lloyd <david.lloyd(a)redhat.com&gt; wrote: > Also be aware that JBMAR has specific optimizations for the JDK empty > collections - they are represented by a single byte in the stream. Well, it's actually 3 bytes that you use to represent empty collections (at least in 1.3.15ga): write(ID_NEW_OBJECT); write(ID_CLASS_CLASS); write(classByte); Whereas with our externalizers, these get resolved via object writer (which gets called before your code to detect empty collections) and it only writes 1 byte: write(externalizerId);

On Oct 22, 2012, at 3:40 PM, David M. Lloyd <david.lloyd(a)redhat.com&gt; wrote: > On 10/22/2012 01:41 AM, Galder Zamarreño wrote: >> >> On Oct 19, 2012, at 7:20 PM, David M. Lloyd <david.lloyd(a)redhat.com&gt; wrote: >> >>> Also be aware that JBMAR has specific optimizations for the JDK empty >>> collections - they are represented by a single byte in the stream. >> >> Well, it's actually 3 bytes that you use to represent empty collections (at least in 1.3.15ga): >> >> write(ID_NEW_OBJECT); >> write(ID_CLASS_CLASS); >> write(classByte); >> >> Whereas with our externalizers, these get resolved via object writer (which gets called before your code to detect empty collections) and it only writes 1 byte: >> >> write(externalizerId); > > No, that's inaccurate; they are singletons and do not use ID_NEW_OBJECT. Really? https://github.com/dmlloyd/jboss-marshalling/blob/1.3.15.GA/river/src/mai...

> They're done as instances so it is one of: > > public static final int ID_EMPTY_LIST_OBJECT = 0x5d; > public static final int ID_EMPTY_SET_OBJECT = 0x62; > public static final int ID_EMPTY_MAP_OBJECT = 0x69; > > Your externalizers on the other hand take multiple bytes even in the > simplest case. If it is that important, you could use an ObjectResolver > to swap in the JDK instance instead of using yet another externalizer > (which has several bytes of overhead of its own). Well, to be completely precise, our version will write two bytes: 1st byte: https://github.com/dmlloyd/jboss-marshalling/blob/1.3.15.GA/river/src/mai... 2nd byte: https://github.com/galderz/infinispan/blob/t_memory/core/src/main/java/or... I still see 3 bytes written in your case: 1st byte: https://github.com/dmlloyd/jboss-marshalling/blob/1.3.15.GA/river/src/mai... 2nd byte: https://github.com/dmlloyd/jboss-marshalling/blob/1.3.15.GA/river/src/mai... 3rd byte: class byte, as indicated in https://github.com/dmlloyd/jboss-marshalling/blob/1.3.15.GA/river/src/mai... Not sure if you remember but we have ObjectTable.Writer in what we call ExternalizerTable, which given a type, it resolves into an Infinispan Externalizer. Now, given Collections.Empty<Set>, it will resolve to an adapter that writes the ID of the externalizer (in this case InfinsipanCollections.EmptySet.Externalizer), and then will delegate to its writeObject, which does nothing. > > -- > - DML > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño galder(a)redhat.com twitter.com/galderz Project Lead, Escalante http://escalante.io Engineer, Infinispan http://infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Oct 19, 2012, at 6:28 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > Galder, what JDK are you using? java version "1.6.0_35" Java(TM) SE Runtime Environment (build 1.6.0_35-b10-428-11M3811) Java HotSpot(TM) 64-Bit Server VM (build 20.10-b01-428, mixed mode) > OpenJDK 1.7 uses EmptyIterator.EMPTY_ITERATOR since 2007: > > http://hg.openjdk.java.net/jdk7/jdk7/jdk/annotate/37a05a11f281/src/share/... Good that the figured out this is non-sense. > I don't have JDK 1.6 sources on hand to check, but I don't think it's worth optimizing for such an old version anyway. Why not? We support JDK 1.6, so nothing stops them from running it. Until we baseline on JDK7 (or 8), I'd leave it in.

> Cheers > Dan > > > On Fri, Oct 19, 2012 at 5:50 PM, Vladimir Blagojevic < vblagoje(a)redhat.com&gt; wrote: > Cool! Did not know about this! Is this original idea or others are > already doing this? > On 12-10-19 8:31 AM, Galder Zamarreño wrote: > > Hi all, > > > > Re: https://github.com/galderz/infinispan/commit/0609207d13216de81d77ff51dc20... > > > > Please avoid using these JDK methods where possible. I've created alternative versions in InfinispanCollections util class that provide a better implementation. > > > > The problem with the JDK versions is that even if these collections are immutable, if you wanna iterate them (i.e. for(X : emptySet) ), they'll create a brand new Iterator every time they're looped, and that generates useless garbage. > > > > The InfinispanCollections versions of emptyX will return a constant singleton iterator which avoids this problem, and avoids the need for client code to check if the collection is empty before looping. > > > > Cheers, > > -- > > Galder Zamarreño > > galder(a)redhat.com > > twitter.com/galderz > > > > Project Lead, Escalante > > http://escalante.io > > > > Engineer, Infinispan > > http://infinispan.org > >

On 22 Oct 2012, at 09:13, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: On Mon, Oct 22, 2012 at 9:25 AM, Galder Zamarreño <galder(a)redhat.com&gt;wrote: > > On Oct 19, 2012, at 6:28 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > Galder, what JDK are you using? > > java version "1.6.0_35" > Java(TM) SE Runtime Environment (build 1.6.0_35-b10-428-11M3811) > Java HotSpot(TM) 64-Bit Server VM (build 20.10-b01-428, mixed mode) > > > OpenJDK 1.7 uses EmptyIterator.EMPTY_ITERATOR since 2007: > > > > > http://hg.openjdk.java.net/jdk7/jdk7/jdk/annotate/37a05a11f281/src/share/... > > Good that the figured out this is non-sense. > > > I don't have JDK 1.6 sources on hand to check, but I don't think it's > worth optimizing for such an old version anyway. > > Why not? We support JDK 1.6, so nothing stops them from running it. > > Until we baseline on JDK7 (or 8), I'd leave it in. > > My point was that it is a really big change, Why? The patch was trivial. Implementing it is easy, and then doing a search and replace on the codebase again resulted in a pretty small patch. and further it imposes a burden on anyone writing code with collections in Infinispan. Is it that hard for a reviewer to spot? :) Maybe even write a script to check and warn for this.

out places there the author should have used InfinispanCollections instead of Collections... I would be willing to go for it if a benchmark showed a significant increase in performance after the change, but you didn't mention any benchmarks, so I guessed that you didn't see a big difference... > > > Cheers > > Dan > > > > > > On Fri, Oct 19, 2012 at 5:50 PM, Vladimir Blagojevic < > vblagoje(a)redhat.com&gt; wrote: > > Cool! Did not know about this! Is this original idea or others are > > already doing this? > > On 12-10-19 8:31 AM, Galder Zamarreño wrote: > > > Hi all, > > > > > > Re: > https://github.com/galderz/infinispan/commit/0609207d13216de81d77ff51dc20... > > > > > > Please avoid using these JDK methods where possible. I've created > alternative versions in InfinispanCollections util class that provide a > better implementation. > > > > > > The problem with the JDK versions is that even if these collections > are immutable, if you wanna iterate them (i.e. for(X : emptySet) ), they'll > create a brand new Iterator every time they're looped, and that generates > useless garbage. > > > > > > The InfinispanCollections versions of emptyX will return a constant > singleton iterator which avoids this problem, and avoids the need for > client code to check if the collection is empty before looping. > > > > > > Cheers, > > > -- > > > Galder Zamarreño > > > galder(a)redhat.com > > > twitter.com/galderz > > > > > > Project Lead, Escalante > > > http://escalante.io > > > > > > Engineer, Infinispan > > > http://infinispan.org > > > > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani manik(a)jboss.org twitter.com/maniksurtani Platform Architect, JBoss Data Grid http://red.ht/data-grid _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Oct 22, 2012, at 10:13 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > On Mon, Oct 22, 2012 at 9:25 AM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: > > On Oct 19, 2012, at 6:28 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > Galder, what JDK are you using? > > java version "1.6.0_35" > Java(TM) SE Runtime Environment (build 1.6.0_35-b10-428-11M3811) > Java HotSpot(TM) 64-Bit Server VM (build 20.10-b01-428, mixed mode) > > > OpenJDK 1.7 uses EmptyIterator.EMPTY_ITERATOR since 2007: > > > > http://hg.openjdk.java.net/jdk7/jdk7/jdk/annotate/37a05a11f281/src/share/... > > Good that the figured out this is non-sense. > > > I don't have JDK 1.6 sources on hand to check, but I don't think it's worth optimizing for such an old version anyway. > > Why not? We support JDK 1.6, so nothing stops them from running it. > > Until we baseline on JDK7 (or 8), I'd leave it in. > > > My point was that it is a really big change, and further it imposes a burden on anyone writing code with collections in Infinispan. I'm sure we will spend non-trivial amounts of time during reviews pointing out places there the author should have used InfinispanCollections instead of Collections… It's part of your job as pull request handler to check for these things, and many other bad practices that can be carried out… I don't see why is this different to other. What's missing here is a check list of things we should be checking for when we handle pull requests. I volunteer to do one once I got 2LC out of the way. > I would be willing to go for it if a benchmark showed a significant increase in performance after the change, but you didn't mention any benchmarks, so I guessed that you didn't see a big difference… It's doesn't affect performance directly, but indirectly. It's about memory consumption. If you use less memory to do the same work, less burden on GC, and less time spent doing GC.

> > > > > > Cheers > > Dan > > > > > > On Fri, Oct 19, 2012 at 5:50 PM, Vladimir Blagojevic < vblagoje(a)redhat.com&gt; wrote: > > Cool! Did not know about this! Is this original idea or others are > > already doing this? > > On 12-10-19 8:31 AM, Galder Zamarreño wrote: > > > Hi all, > > > > > > Re: https://github.com/galderz/infinispan/commit/0609207d13216de81d77ff51dc20... > > > > > > Please avoid using these JDK methods where possible. I've created alternative versions in InfinispanCollections util class that provide a better implementation. > > > > > > The problem with the JDK versions is that even if these collections are immutable, if you wanna iterate them (i.e. for(X : emptySet) ), they'll create a brand new Iterator every time they're looped, and that generates useless garbage. > > > > > > The InfinispanCollections versions of emptyX will return a constant singleton iterator which avoids this problem, and avoids the need for client code to check if the collection is empty before looping. > > > > > > Cheers, > > > -- > > > Galder Zamarreño > > > galder(a)redhat.com > > > twitter.com/galderz > > > > > > Project Lead, Escalante > > > http://escalante.io > > > > > > Engineer, Infinispan > > > http://infinispan.org > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño galder(a)redhat.com twitter.com/galderz Project Lead, Escalante http://escalante.io Engineer, Infinispan http://infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Oct 23, 2012, at 3:03 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > On Tue, Oct 23, 2012 at 3:27 PM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: > > On Oct 22, 2012, at 10:13 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > On Mon, Oct 22, 2012 at 9:25 AM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: > > > > On Oct 19, 2012, at 6:28 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > > > Galder, what JDK are you using? > > > > java version "1.6.0_35" > > Java(TM) SE Runtime Environment (build 1.6.0_35-b10-428-11M3811) > > Java HotSpot(TM) 64-Bit Server VM (build 20.10-b01-428, mixed mode) > > > > > OpenJDK 1.7 uses EmptyIterator.EMPTY_ITERATOR since 2007: > > > > > > http://hg.openjdk.java.net/jdk7/jdk7/jdk/annotate/37a05a11f281/src/share/... > > > > Good that the figured out this is non-sense. > > > > > I don't have JDK 1.6 sources on hand to check, but I don't think it's worth optimizing for such an old version anyway. > > > > Why not? We support JDK 1.6, so nothing stops them from running it. > > > > Until we baseline on JDK7 (or 8), I'd leave it in. > > > > > > My point was that it is a really big change, and further it imposes a burden on anyone writing code with collections in Infinispan. I'm sure we will spend non-trivial amounts of time during reviews pointing out places there the author should have used InfinispanCollections instead of Collections… > > It's part of your job as pull request handler to check for these things, and many other bad practices that can be carried out… I don't see why is this different to other. > > What's missing here is a check list of things we should be checking for when we handle pull requests. > > I volunteer to do one once I got 2LC out of the way. > > > I would be willing to go for it if a benchmark showed a significant increase in performance after the change, but you didn't mention any benchmarks, so I guessed that you didn't see a big difference… > > It's doesn't affect performance directly, but indirectly. > > It's about memory consumption. If you use less memory to do the same work, less burden on GC, and less time spent doing GC. > > > How much memory does Infinispan allocate for each put/get and how much of that is used by these iterators? Since we already use null instead of empty collections in lots of places, I bet it's not even 0.01%. The coding change is based on information gathered via https://issues.jboss.org/browse/ISPN-2414 - feel free to browse through it and I can upload the snapshot if needed.

> So yeah, these iterators do have an impact, but that impact is negligible, whereas the impact on the developers and reviewers to implement this change is not going to be negligible. Impact on developers/reviewers? Aren't you exagerating a bit? :)

> Of course, I might be wrong and this patch might improve throughput for local caches by 1%, but based on what I know so far that's not the case. > > > > > > > > > > > > Cheers > > > Dan > > > > > > > > > On Fri, Oct 19, 2012 at 5:50 PM, Vladimir Blagojevic < vblagoje(a)redhat.com&gt; wrote: > > > Cool! Did not know about this! Is this original idea or others are > > > already doing this? > > > On 12-10-19 8:31 AM, Galder Zamarreño wrote: > > > > Hi all, > > > > > > > > Re: https://github.com/galderz/infinispan/commit/0609207d13216de81d77ff51dc20... > > > > > > > > Please avoid using these JDK methods where possible. I've created alternative versions in InfinispanCollections util class that provide a better implementation. > > > > > > > > The problem with the JDK versions is that even if these collections are immutable, if you wanna iterate them (i.e. for(X : emptySet) ), they'll create a brand new Iterator every time they're looped, and that generates useless garbage. > > > > > > > > The InfinispanCollections versions of emptyX will return a constant singleton iterator which avoids this problem, and avoids the need for client code to check if the collection is empty before looping. > > > > > > > > Cheers, > > > > -- > > > > Galder Zamarreño > > > > galder(a)redhat.com > > > > twitter.com/galderz > > > > > > > > Project Lead, Escalante > > > > http://escalante.io > > > > > > > > Engineer, Infinispan > > > > http://infinispan.org > > > > > > > > _______________________________________________ > > infinispan-dev mailing list > > infinispan-dev(a)lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > -- > Galder Zamarreño > galder(a)redhat.com > twitter.com/galderz > > Project Lead, Escalante > http://escalante.io > > Engineer, Infinispan > http://infinispan.org > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño galder(a)redhat.com twitter.com/galderz Project Lead, Escalante http://escalante.io Engineer, Infinispan http://infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Tue 2012-10-23 22:42, Dan Berindei wrote: > I was probably overreacting, but I just wouldn't want us to end up with > rules like 'all method parameters must be final' ;-) Well, that's good practice anyways, you should enforce that. I know we do in the Hibernate team.

On Tue, Oct 23, 2012 at 11:42 PM, Emmanuel Bernard <emmanuel(a)hibernate.org&gt;wrote: > On Tue 2012-10-23 22:42, Dan Berindei wrote: > > I was probably overreacting, but I just wouldn't want us to end up with > > rules like 'all method parameters must be final' ;-) > > Well, that's good practice anyways, you should enforce that. I know we > do in the Hibernate team. > Oops :) I'm not saying it's bad, and maybe if the code base started this way it's even easy to manage. But it's a matter of style, not a matter of performance, and I definitely wouldn't want us to start enforcing it in Infinispan.