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