[infinispan-dev] Avoid Collections.emptySet() / emptyMap() / emptyList

Tue Oct 23 15:42:03 EDT 2012

On Tue, Oct 23, 2012 at 6:09 PM, Galder Zamarreño <galder at redhat.com> wrote:

>
> On Oct 23, 2012, at 3:03 PM, Dan Berindei <dan.berindei at gmail.com> wrote:
>
> > On Tue, Oct 23, 2012 at 3:27 PM, Galder Zamarreño <galder at redhat.com>
> wrote:
> >
> > On Oct 22, 2012, at 10:13 AM, Dan Berindei <dan.berindei at gmail.com>
> wrote:
> >
> > > On Mon, Oct 22, 2012 at 9:25 AM, Galder Zamarreño <galder at redhat.com>
> wrote:
> > >
> > > On Oct 19, 2012, at 6:28 PM, Dan Berindei <dan.berindei at 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.
>
>
Sorry, I only read the subject line of ISPN-2414 - I thought it was a
different issue because these iterators are transient so they wouldn't
count for Infinispan's memory usage.

1.7% of the memory allocations just for the EmptySet iterator does sound
like a lot, did you also compare performance before and after your change?

> > 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? :)
>
>
Well, I was convinced that the impact of these changes on performance was
0, and anything is greater than 0, even doing a search for
'Collections.empty' in the PR diff.

I was probably overreacting, but I just wouldn't want us to end up with
rules like 'all method parameters must be final' ;-)

> > 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 at 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
> > > > > galder at redhat.com
> > > > > twitter.com/galderz
> > > > >
> > > > > Project Lead, Escalante
> > > > > http://escalante.io
> > > > >
> > > > > Engineer, Infinispan
> > > > > http://infinispan.org
> > > > >
> > >
> > > _______________________________________________
> > > infinispan-dev mailing list
> > > infinispan-dev at lists.jboss.org
> > > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >
> >
> > --
> > Galder Zamarreño
> > galder at redhat.com
> > twitter.com/galderz
> >
> > Project Lead, Escalante
> > http://escalante.io
> >
> > Engineer, Infinispan
> > http://infinispan.org
> >
> >
> > _______________________________________________
> > infinispan-dev mailing list
> > infinispan-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >
> > _______________________________________________
> > infinispan-dev mailing list
> > infinispan-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Galder Zamarreño
> galder at redhat.com
> twitter.com/galderz
>
> Project Lead, Escalante
> http://escalante.io
>
> Engineer, Infinispan
> http://infinispan.org
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20121023/007ce2a7/attachment-0001.html 