On 11/1/15 10:18 AM, Alan Bateman wrote:
On 30/10/2015 21:30, Xueming Shen wrote:
Hi,
Thanks for the comments/suggestions. Here are the updated webrevs
with minor changes here
and there based on the feedback.
I spent time going through the changes in jdk repo and it looks very
good.
One thing that isn't clear to me is why StringUTF16 needs a native
method to test for big endian. Could it use Unsafe::isBigEndian or is
there a non-obvious bootstrapping/initialization issue here?
unsafe.putChar/getChar() were used in String (they are still being used
in charset changes).
http://cr.openjdk.java.net/~sherman/8054307/jdk.1030/src/java.base/share/classes/java/lang/StringUTF16.java.html
However it triggered a vm failure in hotspot PIT, in which it appears
the unsafe is not ready/loaded
at certain circumstance.
...results/workDir/demo/jvmti/heapTracker/HeapTrackerTest/hs_err_pid12542.log
So we are back to the old approach to go get the endian ourselves.
The additional test coverage looks quite good but I'm guessing many of
these tests overlap with existing tests. Are you planning to leave
them in the CompactStrings directory? I assume it would make more
sense to move them up to the String directory. I was also wondering if
more of the existing tests should be updated to run them both ways. In
passing, SerializationUtils has a semi-colon at L47 that looks a bit odd.
Yes, it might be worth to take some time to re-organize the String
tests. We can address it in
a separate follow-up bug fix. Need take to the SQE team.
Thanks,
-Sherman
