[lworld] RFR: object methods in VM for lworld value type (original) (raw)

John Rose john.r.rose at oracle.com
Tue May 8 06:19:20 UTC 2018

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/objectmethods/webrev1/ <http://cr.openjdk.java.net/~dsimms/valhalla/objectmethods/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

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

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.

More information about the valhalla-dev mailing list