RFR 8145628: hotspot metadata classes shouldn't use HeapWordSize or heap related macros like align_object_size
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Jan 28 21:29:10 UTC 2016
Jon,
Thank you for reviewing.
On 1/28/16 3:41 PM, Jon Masamitsu wrote:
> Changes look good. Some minor points (for which I don't
> need to see another webrev if you decide to make changes
> based on these)
>
> http://cr.openjdk.java.net/~coleenp/8145628.01/webrev/src/share/vm/oops/klassVtable.hpp.udiff.html
>
>
> - static int size() { return sizeof(itableOffsetEntry) / HeapWordSize;
> } // size in words
> + static int size() { return sizeof(itableOffsetEntry)/wordSize; } //
> size in words
>
> Surrounding code puts space around the operators. In this case around
> the " / " seem
> appropriate.
The spacing around sizeof(Blah)/wordSize is inconsistent in the oops
directory. My preference is no spaces. But there was another instance
that I'd missed for vtableEntry:
- static int size() {
- return sizeof(vtableEntry) / sizeof(HeapWord);
- }
+ static int size() { return sizeof(vtableEntry) /
wordSize; }
I added the spaces because you were nice enough to review my code.
>
>
> http://cr.openjdk.java.net/~coleenp/8145628.01/webrev/src/share/vm/utilities/globalDefinitions.cpp.frames.html
>
>
> You added
>
> 88 assert(wordSize == BytesPerWord, "should be the same since they're
> used interchangeably");
>
> Many of the changes were like the one above in klassVtable.hpp where
> HeapWordSize -> wordSize.
> Do you want to assert "wordSize == HeapWordSize"?
Added:
assert(wordSize == BytesPerWord, "should be the same since they're
used interchangeably");
assert(wordSize == HeapWordSize, "should be the same since they're
also used interchangeably");
Thanks!
Coleen
>
> Jon
>
>
>
> On 01/27/2016 10:27 AM, Coleen Phillimore wrote:
>> Summary: Use align_metadata_size, align_metadata_offset and
>> is_metadata_aligned for metadata rather
>> than align_object_size, etc. Use wordSize rather than HeapWordSize
>> for metadata. Use align_ptr_up
>> rather than align_pointer_up (all the related functions are ptr).
>>
>> Ran RBT quick tests on all platforms along with Chris's Plummers
>> change for 8143608, ran jtreg hotspot tests and nsk.sajdi.testlist
>> co-located tests because there are SA changes. Reran subset of this
>> after merging.
>>
>> I have a script to update copyrights on commit. It's not a big
>> change, just mostly boring. See the bug comments for more details
>> about the change.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8145628.01/
>> bug link https://bugs.openjdk.java.net/browse/JDK-8145628
>>
>> thanks,
>> Coleen
>>
>>
>
More information about the hotspot-dev
mailing list