On Oct 23, 2012, at 3:03 PM, Dan Berindei <dan.berin...@gmail.com> wrote:
> On Tue, Oct 23, 2012 at 3:27 PM, Galder Zamarreño <gal...@redhat.com> wrote: > > On Oct 22, 2012, at 10:13 AM, Dan Berindei <dan.berin...@gmail.com> wrote: > > > On Mon, Oct 22, 2012 at 9:25 AM, Galder Zamarreño <gal...@redhat.com> wrote: > > > > On Oct 19, 2012, at 6:28 PM, Dan Berindei <dan.berin...@gmail.com> 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/classes/java/util/Collections.java > > > > 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 > > > <vblag...@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/0609207d13216de81d77ff51dc20652ce270c635 > > > > > > > > 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 > > > > gal...@redhat.com > > > > twitter.com/galderz > > > > > > > > Project Lead, Escalante > > > > http://escalante.io > > > > > > > > Engineer, Infinispan > > > > http://infinispan.org > > > > > > > > _______________________________________________ > > infinispan-dev mailing list > > infinispan-dev@lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > -- > Galder Zamarreño > gal...@redhat.com > twitter.com/galderz > > Project Lead, Escalante > http://escalante.io > > Engineer, Infinispan > http://infinispan.org > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño gal...@redhat.com twitter.com/galderz Project Lead, Escalante http://escalante.io Engineer, Infinispan http://infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
