[infinispan-dev] Allocation costs of TypeConverterDelegatingAdvancedCache

[William Burns]

On Thu, Jun 1, 2017 at 7:11 AM Dan Berindei <dan.berindei at gmail.com> wrote:

> On Wed, May 31, 2017 at 10:05 PM, William Burns <mudokonman at gmail.com>
> wrote:
> > Let me explain why it is there first :) This class was added for two main
> > reasons: as a replacement for compatibility and for supporting equality
> of
> > byte[] object. What this class does is at the user side is box the given
> > arguments (eg. byte[] -> WrappedByteArray) then the cache only ever deals
> > with the boxed types and then does the unboxing for any values that are
> > returned.
> >
> > There are some exceptions with how the boxing/unboxing works for both
> cases
> > such as Streams and Indexing which have to rebox the data to work
> properly.
> > But the cost is pretty minimal.
> >
> > While compatibility is always on or off unfortunately anyone can pass in
> a
> > byte[] at any point for a key or value. So we need to have these wrappers
> > there to make sure they work properly.
> >
> > We could add a option to the cache, which people didn't show interest
> > before, to have a cache that doesn't support byte[] or compatibility. In
> > this case there would be no need for the wrapper.
> >
> >
> > Compatibility
> >
> > By using the wrapper around the cache, compatibility becomes quite
> trivial
> > since we just need a converter in the wrapper and it does everything else
> > for us. My guess is the new encoding changes will utilize these wrapper
> > classes as well as they are quite easy to plug in and have things just
> work.
> >
> > Equality
> >
> > With the rewrite for eviction we lost the ability to use custom Equality
> in
> > the data container. The only option for that is to wrap a byte[] to
> provide
> > our own equality. Therefore the wrapper does this conversion for us by
> > converting between automatically.
> >
> > On Wed, May 31, 2017 at 2:34 PM Sanne Grinovero <sanne at infinispan.org>
> > wrote:
> >>
> >> Hi all,
> >>
> >> I've been running some benchmarks and for the fist time playing with
> >> Infinispan 9+, so please bear with me as I might shoot some dumb
> >> questions to the list in the following days.
> >>
> >> The need for TypeConverterDelegatingAdvancedCache to wrap most
> >> operations - especially "convertKeys" - is highlighet as one of the
> >
> >
> > It should be wrapping every operation pretty much.
> >
> > Unfortunately the methods this hurts the most are putAll, getAll etc as
> they
> > have to not only box every entry but copy them into a new collection as
> you
> > saw in "convertKeys". And for getAll it also has to unbox the return as
> > well.
> >
> > We could reduce allocations in the collection methods by not creating the
> > new collection until we run into one key or value that required
> > boxing/unboxing. This would still require fully iterating over the
> > collection at best case. This should perform well in majority of cases
> as I
> > would expect all or almost all entries either require or don't require
> the
> > boxing. The cases that would be harmed most would be ones that have a
> sparse
> > number of entries that require boxing.
> >
>
> +1 for checking the keys/values first and only creating a new
> collection if necessary, although it doesn't seem possible with the
> current TypeConverter interface.
>

What I have done in the past is that the TypeConverter just returns the
object as is if it didn't change anything. This way you can do instance
equality to verify if there were changes.


>
> I'm also fine with adding a configuration option to prohibit byte[]
> keys/values, but I'd still want to check the collection and throw an
> exception (at least for writes).
>
> Thinking further, do we really need to wrap byte[] values?
> DataContainer only has a compute() method, so we may be able to
> replace the value wrappers with a TypeConverter.valuesEqual(a, b)
> method.
>

I don't think we can work around it. The key also has to have the same
hashCode and equals and we would then have to wrap it elsewhere to have it
work properly with the underlying ConcurrentHashMap.


>
> Also, Sanne, could you change InternalCacheFactory.createAndWire() to
> return the un-wrapped cache, and then see exactly much the wrapping
> affects performance? With so many moving parts, I wouldn't be
> surprised if the difference in numbers was < 1%.
>
> >>
> >> high allocators in my Search-centric use case.
> >>
> >> I'm wondering:
> >>  A - Could this implementation be improved?
> >
> >
> > Most anything can be improved :) The best way would be to add another
> knob.
> >
> >>  B - Could I bypass / disable it? Not sure why it's there.
> >
> >
> > There is no way to bypass currently. Explained above.
> >
> >
> >>
> >>
> >> Thanks,
> >> Sanne
> >> _______________________________________________
> >> 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
> _______________________________________________
> 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/20170601/14a97bc5/attachment.html