RFR (S): 8079091: Remove dictionary NULL check on common path of BlockFreeList methods

[Jon Masamitsu]

On 4/30/2015 1:56 AM, David Holmes wrote:
> On 30/04/2015 5:37 PM, Jungwoo Ha wrote:
>> My original change (not embedding CHeapObj):
>> http://cr.openjdk.java.net/~jwha/8079091/webrev.00/
>> Jon's suggestion (from hotspot-gc-dev):
>> http://cr.openjdk.java.net/~jwha/8079091/webrev.02/
>> <http://cr.openjdk.java.net/~jwha/8079091/webrev.00/>
>>
>> Please take a look on .02.
>
> That seems reasonable, but someone more familiar with metaspace needs 
> to comment on why _dictionary was initially lazily-initialized.

There nothing clever in the lazily-initialized.  I should have had more 
faith
in the initialization of the class.

Jon

>
>> On Thu, Apr 30, 2015 at 12:05 AM, Jungwoo Ha <jwha at google.com
>> <mailto:jwha at google.com>> wrote:
>>
>>     Good point.
>>     However, it seems that ChunkManager also has the ChunkTreeDictionary
>>     _humongous_dictionary embedded.
>
> Seems there is confusion over allocation strategies - possibly mine of 
> course.
>
> Thanks,
> David
>
>
>>     My original fix was keeping it as a pointer and allocate it during
>>     BlockFreelist constructor.
>>     See: http://cr.openjdk.java.net/~jwha/8079091/webrev.00/
>>
>>
>>     On Wed, Apr 29, 2015 at 11:20 PM, David Holmes
>>     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>
>>         On 30/04/2015 4:05 PM, Jungwoo Ha wrote:
>>
>>             http://cr.openjdk.java.net/~jwha/8079091/webrev.01
>>
>>
>>         I may be missing something but you seem to have embedded an
>>         object that is defined to be CHeapObj !
>>
>>         David
>>
>>
>>             PTAL.
>>
>>             On Wed, Apr 29, 2015 at 10:22 PM, Jungwoo Ha
>>             <jwha at google.com <mailto:jwha at google.com>> wrote:
>>
>>                 The slower ones have higher error bars (error bars are
>>                 95% confidence
>>                 interval), and you can see that those are not
>>                 statistically significant.
>>                 I'll reflect your suggestions and upload the new webrev.
>>
>>                 On Wed, Apr 29, 2015 at 10:00 PM, Kim Barrett
>>                 <kim.barrett at oracle.com <mailto:kim.barrett at oracle.com>>
>>                 wrote:
>>
>>                     On Apr 29, 2015, at 7:34 PM, Jungwoo Ha
>>                     <jwha at google.com <mailto:jwha at google.com>> wrote:
>>
>>
>>                         BUG:
>> https://bugs.openjdk.java.net/browse/JDK-8079091
>>                         Webrev:
>> http://cr.openjdk.java.net/~jwha/8079091/webrev.00/
>>
>>                         Can someone sponsor this change?
>>                         I've seen ~3% speed up on DaCapo benchmarks.
>>                         (results attached on the
>>
>>                     bug
>>
>>                         page.)
>>
>>
>>                     Wow!
>>
>>                     Hm.  There are a couple of benchmarks that are a
>>                     little bit slower.  I
>>                     don't know enough about that suite of benchmarks to
>>                     know how important
>>                     that might be.
>>
>>                     Since the dictionary is always being created at
>>                     construction time, the
>>                     null check in ~BlockFreelist is no longer needed 
>> either.
>>
>>                     It seems like a further improvement might be
>>                     obtained by making the
>>                     dictionary a direct member subobject of
>>                     BlockFreelist, rather than
>>                     allocating it as a separate object, thereby
>>                     eliminating an
>>                     indirection.
>>
>>                     Pre-existing: Why is there a cast in
>>                     SpaceManager::block_freelists()?
>>
>>                     Pre-existing: I found the private
>>                     BlockFreelist::dictionary() member
>>                     function distracting rather than helpful, and would
>>                     have preferred
>>                     just using the private data member directly. But
>>                     maybe that's just me.
>>                     If _dictionary is changed to a subobject rather than
>>                     a pointer, I'd
>>                     suggest changing a bunch of "dictionary()->XXX" to
>>                     "_dictionary.XXX".
>>
>>
>>
>>
>>