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

[Dan Berindei]

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%.

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20121023/255b8c9d/attachment.html