On 2019-08-16 00:59, Kim Barrett wrote:
On Aug 15, 2019, at 7:46 AM, Stefan Karlsson <stefan.karls...@oracle.com> wrote:
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:
Here's my comments through webrev.valueMarkWord.03.
Looks good. There are a couple of small fixes for which I don't need
a new webrev, which are listed first below. Then there are some
broader items which could be addressed in followup improvements.
------------------------------------------------------------------------------
src/hotspot/share/oops/markOop.hpp
109 template<typename T> operator T();
My mistake in the earlier review comment. Function should be const
qualified, e.g. that should be
template<typename T> operator T() const;
I added this after one of our earlier discussions. However, I don't
think we need it (const or not). We already get sensible compiler errors
without this function when we try to cast markWords to something else:
void* p0 = m;
void* p1 = (void*)m;
int i0 = m;
int i1 = (int)m;
error: cannot convert ‘markWord’ to ‘void*’ in initialization
void* p0 = m;
^
error: invalid cast from type ‘markWord’ to type ‘void*’
void* p1 = (void*)m;
^
error: cannot convert ‘markWord’ to ‘int’ in initialization
int i0 = m;
^
error: invalid cast from type ‘markWord’ to type ‘int’
int i1 = (int)m;
The poisoned constructor seems to be unnecessary as well, now that we
have simplified markWord. Without it, I get appropriate error messages
when I try to create a markWord from a pointer:
error: invalid conversion from ‘void*’ to ‘uintptr_t’ {aka ‘long
unsigned int’} [-fpermissive]
markWord p((void*)0x111);
^~~~~~~~~~~~
note: initializing argument 1 of ‘markWord::markWord(uintptr_t)’
explicit markWord(uintptr_t value) : _value(value) { }
I've removed both of these.
------------------------------------------------------------------------------
src/hotspot/share/runtime/biasedLocking.cpp
695 prototype.bias_epoch() ==
mark.bias_epoch())) {
I think one more leading space needs to be deleted to get proper
alignment here. Or reformat this long and complex if control
expression.
OK. I followed the pre-existing alignment, but I can change it anyway.
------------------------------------------------------------------------------
src/hotspot/share/runtime/vframe.cpp
244 // FIXME: mark is set but not used below.
245 // Either the comment or the code is
broken.
Is there a bug for this?
Created JDK-8229808.
------------------------------------------------------------------------------
The remainder seem like they could be followup improvements.
------------------------------------------------------------------------------
src/hotspot/share/oops/markOop.hpp
138 static const uintptr_t zero = 0;
All occurrences of this are either
(1) markWord member initializater
(2) markWord variable initializer
(3) temporary markWord initializer
There don't appear to be any bare uses otherwise. I think nicer is
static markWord zero() { return markWord(0); }
(For C++11: `static constexpr markWord zero = markWord(0);`)
This seems like it could be a followup improvement.
I had the same thought and then backed away from it, but I can't
remember why. This is a small enough change, so I've gone through the
few occurrences and cleaned it up.
I'll leave the rest of the comments below for follow-up RFEs.
This is the last few cleanups:
http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04.delta/
http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04/
I ran extended testing on .03 (tier1-7 on linux), checked the markWord
functions were inlined, and checked that the generated code for
G1ParScanThreadState::copy_to_survivor_space was the same before and
after the patch. So I intend to run tier1 testing on .04 and then push
this patch.
Thanks,
StefanK
------------------------------------------------------------------------------
[This is also related to Coleen's comments about to_pointer.]
Looking at the changes, in order to reduce the amount of casting pixie
dust sprinkled on our code base, I now think markWord::to_pointer
should have a template overload, with the template parameter
designating the return type. And I think the return type for the
non-template should be const qualified, and the template should handle
cv qualifiers. Like so (I think, I haven't actually tested this)
const void* to_pointer() const {
return reinterpret_cast<void*>(_value);
}
template<typename T>
T* to_pointer() const {
typedef typename RemoveCV<T>::type TT;
return reinterpret_cast<TT*>(_value);
}
If one wants a void* then use m.to_pointer<void>().
(I almost want to call it "pointer_to" so it reads "pointer to T".)
Coleen and I talked about a possible alternative to the template
overload. Perhaps there is a small and fixed number of pointer types
we want to support conversion to, in which case we could have a small
number of type-specific to_xxx_pointer variants? But I'm not sure the
number is actually small and fixed.
This seems like it could be a followup improvement.
------------------------------------------------------------------------------
src/hotspot/share/interpreter/bytecodeInterpreter.cpp
In the run() function, there are a lot of casts of markWord::value()
results or constants from markWord that I think could be removed.
Some of them might want C++11 explicitly typed enums though.
This seems like it could be a followup improvement.
------------------------------------------------------------------------------
