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

[David Holmes]

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.

> 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".
>
>
>
>
>