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

[John Rose]

On May 7, 2018, at 6:21 AM, David Simms <david.simms at oracle.com> wrote:
> 
> Updated webrev: http://cr.openjdk.java.net/~dsimms/valhalla/object_methods/webrev1/ <http://cr.openjdk.java.net/~dsimms/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?

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

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…

# 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 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).

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!

# 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 &&.

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

(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.)

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.