[lworld] RFR: object methods in VM for lworld value type

[David Simms]

Inline...

On 8/05/2018 8:19 a.m., John Rose wrote:
> On May 7, 2018, at 6:21 AM, David Simms <david.simms at oracle.com 
> <mailto:david.simms at oracle.com>> wrote:
>>
>> Updated 
>> webrev:http://cr.openjdk.java.net/~dsimms/valhalla/object_methods/webrev1/ 
>> <http://cr.openjdk.java.net/%7Edsimms/valhalla/object_methods/webrev1/>
>
> More comments:
>
> I love how so much is built on top of the simple but deep hack of
> KlassPtrValueTypeMask.  That was the right move.
>
> Still, I think the markOop of a buffered value should *also* have
> a distinctive bit pattern.  Can we do this also?
>

Sure metadata is static, buffering decision and its internal state is 
dynamic, mark word would be the right place for that.

> # macroAssembler_x86.cpp
>
> +#ifdef _LP64
> +  if (UseCompressedClassPointers) {
> +    movl(temp, Address(oop, oopDesc::klass_offset_in_bytes()));
> +  } else
> +#endif
> +  movptr(temp, Address(oop, oopDesc::klass_offset_in_bytes()));
>
> It's about time to add an overloads to control the #ifdef madness.
> Maybe have a decorator enum to discriminate uses:
>   enum PtrKind { Word, Oop, Klass };
> and then:
>   void movptr(PtrKind k, Register d, Address a);
>
> The assembler would look at k and then at any config flags, and
> then decide whether movl or movq were the move du jour.  This is
> worth a follow-up bug, so we can do it in the main line first.
>

Yeah, I'd like to defer until we have a clearer picture of what have, 
and yeah break out generic stuff into main-line as much as possible. 
There are also a number of code re-factorizations to be look at in the 
oop/klass hierarchy which fall into the same category (i.e. we started 
with copy, paste, tweak, tweak...but the end result can be unified).

> If you have to pick a polarity, I suppose "not a value" is the thing you
> want to test, and this works out OK.  But there's a way to test both ways:
>
> void test_oop_is_value(Register oop, Register temp, Label* is_value, 
> Label* is_not_value);
>
> The idea is that one label can be NULL, and that refers to fall-through.
> The jcc at the end of the macro jumps to the non-null label with the 
> right cc.
> Some assembler macros are polarity-agnostic in this way.  Just a thought…
>

Sure thing, the negative test is a little weird.

> # templateInterpreterGenerator_x86.cpp, templateTable_x86.cpp
>
> Did you try to put an "already locked" pattern in the header?  That will
> push many tests related to synchronization down onto the slow path.
>

I did play with already locked, but did not like it much. That was 
before we decided hashCode could be an BSM story, I thought we might 
need mark word for caching hash, not so anymore. However, having a 
sentinel fat monitor or biased to fake thread coupled with seems like it 
might collide with "buffered oops" if we need mark word for state 
there...although we need to rework buffering somewhat, last time we need 
the whole word for buffer relocation (forget if this was actually 
visible)...perhaps "fat locked" or "bias locked" together with 
monitor/thread reference high bit is compatible.

I digress, the real thing that got me to avoid this, was GC "should 
preserve mark word" leading to mark word stack usage (GC needs the mark 
for it's own evil purposes, and needs preserve the value for anything 
"special", which I suspect is uncommon case)...I was concerned what 
extra work this would be, both for implementation and runtime performance.

But I guess I could revisit, since the monitorenter number of comparison 
ops can be reduced. Since the test can be moved to GC preserve/restore 
mark, runtime cost would be a profit.

> I noticed that you test for value types in TemplateTable::monitorenter,
> but this test is not needed if you have a backup test on the slow
> path for all object sync. operations.  All of the throws can be done
> in synchronizer.cpp (which you seem to have covered).

Only if forced to slow by "already locked"...will check it out.

>
> The prototype header should be set to this pattern early on, in
> SystemDictionary::update_dictionary.  For example, a thread ID
> (1 or -1) could be stolen from the biased locking pattern.  A bias
> of this special stolen not-really-a-thread-ID would mean "I'm a
> value".  This thread ID would push all sync. slow paths straight
> into your error handler.
>
> Or, if we don't want to build on top of biased locking, the special
> bit pattern could be defined to look "just like a biased lock", in
> case biased locking is enabled, otherwise it would be a standalone
> pattern which no other state uses (even if it isn't reserved for
> biased locking).
>
> # klass.cpp
>
> Yeah!
>

I've forgotten something, probably array klasses, and AppCDS probably 
can't be completely ignored, like we had been.

> # klass.inline.hpp
>
> +  assert(!header->has_bias_pattern() || (is_instance_klass() && 
> (!is_value())), "biased locking currently only supported for Java 
> instances");
>
> I get the parens around the && term, but parens around the ! term
> smells like over-scrupulousity to me.  There's no chance somebody
> will forget that ! binds more tightly than &&.
>

TBH, not enough parens...actually you are correct the code is 
inconsistent, first "! X ||" vs "&& (! Y)"

> # synchronizer.cpp
>
> Yes, this is where the throws should come from.  In fact, all of them.
>
> Late-breaking suggestion for a default IHC for a value type:  Return
> the IHC of its getClass.  IMO, every non-default hashCode for a
> value type should take into account most or all fields of the value,
> *plus* the getClass of the value (or the getClass.getName).
>
> Rationale:  We don't want Foo(0, 0) and Bar(0, 0) to hash to the
> same hashCode.  We want the Foo vs. Bar distinction to flip bits,
> even if the fields don't.

Sure, why not.

>
> (Side note:  What is the reason we are using FastHashCode as a
> standard entry point?  In whose local style guide is that a good name
> for a HotSpot API point?  Shouldn't we change FastHashCode to
> fast_identity_hash_value?  That would be more in line with global
> style.  Also, FastHashCode doesn't say "identity", which is confusing.
> But I don't know the history of this name.)
>

I removed a dead code path, then "FastHashCode" is what remained, it 
could be renamed, perhaps in main-line (not a Valhalla specific).

> Final comment:  Nice tests!
>
> Good work.
>
> — John
>
> P.S. Is there a path to make frozen arrays be just plain arrays,
> but somehow also values?  And also other kinds of instances.
> This is doable if we lean heavily on the mark word to tell us
> when identity and mutability are disabled.  Doesn't work as
> well if we put all the burden on the klass field, unless we split
> array classes internally into array-value and array-object.
>
>

Yes, a frozen array could be an "array-value", VM implementation would 
be pretty trival, use the same mechanism for aastore / klass encoding / 
"already locked".