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

[Jungwoo Ha]

I agree.
See how _humongous_dictionary is implemented. ChunkManager::print_on has
the same const_cast.
I just tried to be consistent on styles.


On Thu, Apr 30, 2015 at 6:50 AM, Stefan Karlsson <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/
>> 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".
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>