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

[Stefan Karlsson]

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

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> 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/
>>
>>
>> On Wed, Apr 29, 2015 at 11:20 PM, David Holmes <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> 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>
>>>>> wrote:
>>>>>
>>>>>   On Apr 29, 2015, at 7:34 PM, Jungwoo Ha <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".
>>>>>>
>>>>>>
>>>>>>