Hi Roman,
On 2019-08-15 19:06, Roman Kennke wrote:
Hi Stefan,
I looked over the changes again. I like this much better, a huge
improvement over current state, and also better than your first
proposal. I also prefer the explicit value() calls.
Great!
I also built+tested Shenandoah GC again, seems all fine.
Didn't know that C++ has an 'explicit' specifier. Oh man.
Still seems to have foobared alignment (it was partly kaputted before
already):
src/hotspot/share/oops/oopsHierarchy.hpp
You're right. I removed one stray whitespace:
$ hg diff
diff --git a/src/hotspot/share/oops/oopsHierarchy.hpp
b/src/hotspot/share/oops/oopsHierarchy.hpp
--- a/src/hotspot/share/oops/oopsHierarchy.hpp
+++ b/src/hotspot/share/oops/oopsHierarchy.hpp
@@ -46,7 +46,7 @@
typedef class instanceOopDesc* instanceOop;
typedef class arrayOopDesc* arrayOop;
typedef class objArrayOopDesc* objArrayOop;
-typedef class typeArrayOopDesc* typeArrayOop;
+typedef class typeArrayOopDesc* typeArrayOop;
#else
I think the other indentation is done on purpose:
typedef class oopDesc* oop;
typedef class instanceOopDesc* instanceOop;
typedef class arrayOopDesc* arrayOop;
typedef class objArrayOopDesc* objArrayOop;
typedef class typeArrayOopDesc* typeArrayOop;
to show the oops hierarchy.
Out of curiosity, what's with the changes in objectMonitor.inline.hpp to
access the markWord atomically?:
-inline markOop ObjectMonitor::header() const {
- return _header;
+inline markWord ObjectMonitor::header() const {
+ return Atomic::load(&_header);
}
I guess this is good (equal or stronger than before) but is there a
rationale behind these changes?
Ahh. Right. That was done to solve the problems I were having with
volatiles. For example:
src/hotspot/share/runtime/objectMonitor.inline.hpp:38:10: error: binding
reference of type 'const markWord&' to 'const volatile markWord'
discards qualifiers
return _header;
and:
src/hotspot/share/runtime/basicLock.hpp:40:74: error: implicit
dereference will not access object of type ‘volatile markWord’ in
statement [-Werror]
void set_displaced_header(markWord header) {
_displaced_header = header; }
Kim suggested that the fact that these fields were volatile was an
indication that we should be doing some kind of atomic/ordered
operation. By replacing these loads and stores with calls to the Atomic
APIs, and providing the PrimitiveConversions::Translate<markWord>
specialization, we could solve that problem.
I say ship it!
Thanks a lot for reviewing this!
StefanK
Thanks,
Roman
Thanks Kim, Roman, Dan and Coleen for reviews and feedback.
I rebased the patch, fixed more alignments, renamed the bug, and rerun
the test through tier1-3.
https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03.delta/
https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03/
Could I get reviews for this version? I'd also like to ask others to at
least partially look at this:
1) Platform maintainers probably want to run this patch through their
build system.
2) SA maintainers (CC:ed serviceability-dev)
3) JVMCI maintainers
Thanks,
StefanK
On 2019-08-14 11:11, Roman Kennke wrote:
Am 14.08.19 um 01:26 schrieb Kim Barrett:
On Aug 12, 2019, at 12:19 PM, Stefan Karlsson
<stefan.karls...@oracle.com> wrote:
Hi Roman,
Kim helped me figuring out how to get past the volatile issues I had
with the class markWord { uintptr_t value; ... } version. So, I've
created a version with that:
https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.01/
I can go with either approach, so let me now what you all think.
I've finally had time to look at the first proposed change.
Comparing the first approach (an AllStatic MarkWord class and markWord
typedef'd to uintptr_t) vs the second approach (markWord is a thin
class wrapping around uintptr_t), I prefer the second.
* I think the markWord class provides better type safety. It still
involves too many casts sprinkled over the code base, but I think it
also provides a better basis for further cast reduction and
prevention.
* I think having one markWord class for the data and behavior is
better / more natural than having a markWord typedef for the data and
a MarkWord AllStatic class for the behaviour.
* I like that the markWord class eliminates the markWord vs MarkWord
homonyms, which I think will be annoying.
* The markWord class is a trivially copyable class, allowing it to be
efficiently passed around by value, so no disadvantage there.
I haven't found anything that I think argues for the first over the
second. Other folks might have different priorities or taste. I think
either is better than the status quo.
I'm still reviewing webrev.valueMarkWord.02, but so far haven't found
anything that makes me want to suggest backing off from that direction.
Note that the bug summary doesn't describe the second approach.
+1 :-)
Roman
