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

[Stefan Karlsson]

On 2015-04-30 15:56, Jungwoo Ha wrote:
> I agree.
> See how _humongous_dictionary is implemented. ChunkManager::print_on 
> has the same const_cast.
> I just tried to be consistent on styles.

Yes. Your change looks good, given the current style.

Thanks,
StefanK

>
> On Thu, Apr 30, 2015 at 6:50 AM, Stefan Karlsson 
> <stefan.karlsson at oracle.com <mailto:stefan.karlsson at oracle.com>> wrote:
>
>     Hi Jung
>
>     On 2015-04-30 09:37, Jungwoo Ha wrote:
>
>         My original change (not embedding CHeapObj):
>         http://cr.openjdk.java.net/~jwha/8079091/webrev.00/
>         <http://cr.openjdk.java.net/%7Ejwha/8079091/webrev.00/>
>         Jon's suggestion (from hotspot-gc-dev):
>         http://cr.openjdk.java.net/~jwha/8079091/webrev.02/
>         <http://cr.openjdk.java.net/%7Ejwha/8079091/webrev.02/>
>         <http://cr.openjdk.java.net/~jwha/8079091/webrev.00/
>         <http://cr.openjdk.java.net/%7Ejwha/8079091/webrev.00/>>
>
>         Please take a look on .02.
>
>
>     So, the suggested change to use:
>     -  BlockTreeDictionary* _dictionary;
>     +  BlockTreeDictionary _dictionary;
>
>     forced you to remove the const qualifier for dictionary():
>     -  BlockTreeDictionary* dictionary() const { return _dictionary; }
>     +  BlockTreeDictionary* dictionary() { return &_dictionary; }
>
>     which forced you to const_cast away the const so that you could
>     call dictionary() just to then call a const function:
>
>      void BlockFreelist::print_on(outputStream* st) const {
>     -  if (dictionary() == NULL) {
>     -    return;
>     -  }
>     -  dictionary()->print_free_lists(st);
>     +  const_cast<BlockFreelist
>     *>(this)->dictionary()->print_free_lists(st);
>      }
>
>     This starts to look silly. If you used the member variable
>     directly this could have simply been written as:
>     _dictionary->print_free_lists(st);
>
>     Right?
>
>     StefanK
>
>
>
>         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.
>             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/
>             <http://cr.openjdk.java.net/%7Ejwha/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
>                     <http://cr.openjdk.java.net/%7Ejwha/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/
>                                 <http://cr.openjdk.java.net/%7Ejwha/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".
>
>
>
>
>