RFR: 8029795 : LinkedHashMap.getOrDefault() doesn't update access order. (was Why doesn't new Map methods generate entry accesses on LinkedHashMap?)

[Vitaly Davidovich]

I believe C2 compiler is more aggressive about inlining, and will inline
hot methods over the 35 bytes limit.  I guess interpreter and C1 (and other
JVMs, potentially) may get impacted though.  In this case, the issue may be
more about the fact that getOrDefault() is virtual and not so much about
byte code size; devirtualization will need profiling info.  Also, the
inlining method (I.e. get()) would be tiny since it just calls getOrDefault
- if inlining is halted purely on byte code size, that almost smells of a
compiler bug/quality of implementation issue.

Having said that, it's really a shame that such a seemingly small (and
good, from maintenance standpoint) change risks performance degradation.

Sent from my phone
On Dec 10, 2013 10:44 AM, "Mike Duigou" <mike.duigou at oracle.com> wrote:

I considered it but was worried that the combined size would prevent
inlining of the method. The getOrDefault method is currently 33 bytes long
which is right at the common borderline for inlining. It does seem a shame
to duplicate the code but I couldn't be confident I wouldn't degrade
performance by reuse.

(If someone were to write a JMH benchmark to show that get() calling
getOrDefault() was no slower than the naive duplication then it would be
reasonable to consider switching).

Mike


On Dec 9 2013, at 23:22 , Vitaly Davidovich <vitalyd at gmail.com> wrote:

Mike,

Would it make sense to rewrite get() in terms of getOrDefault() to reduce
duplication?

Thanks

Sent from my phone
On Dec 9, 2013 10:40 PM, "Mike Duigou" <mike.duigou at oracle.com> wrote:

> Hello all;
>
> I've posted a webrev for review which corrects the problem and adds
> appropriate tests.
>
> http://cr.openjdk.java.net/~mduigou/JDK-8029795/0/webrev/
>
> I also updated the documentation to mention that getOrDefault as well as
> the replace methods generate access events.
>
> Mike
>
> On Dec 9 2013, at 02:11 , Paul Sandoz <paul.sandoz at oracle.com> wrote:
>
> > Hi Roman,
> >
> > On Dec 8, 2013, at 10:29 PM, Roman Leventov <leventov at ya.ru> wrote:
> >> Especially getDefault(). Doesn't this violate principle of least
> astonishment? Details and proof:
> http://stackoverflow.com/questions/20440136/why-doesnt-new-map-methods-generate-entry-accesses-on-linkedhashmap
> >>
> >
> > Thanks. I believe that all the new (default) Map methods but
> getOrDefault behave correctly. So i think it is a bug, we need to add
> something like the following to LinkedHashMap:
> >
> >    public V getOrDefault(Object key, V defaultValue) {
> >        Node<K,V> e;
> >        if ((e = getNode(hash(key), key)) == null)
> >            return defaultValue;
> >        if (accessOrder)
> >            afterNodeAccess(e);
> >        return e.value;
> >    }
> >
> > and also update the documentation to clarify (via the implementation
> specification of the default methods it can be inferred what the behaviour
> is, but that ain't obvious).
> >
> > I have logged this bug:
> >
> >  https://bugs.openjdk.java.net/browse/JDK-8029795
> >
> > Paul.
> >
> >
> >
>
>