RFR: 8233497: Optimize default method generation by data structure reuse

[Lois Foltan]

On 11/15/2019 10:20 AM, Claes Redestad wrote:
>
>
> On 2019-11-15 15:49, Lois Foltan wrote:
>> On 11/8/2019 6:57 AM, Claes Redestad wrote:
>>> Hi,
>>>
>>> when loading classes with complex hierarchies and many default methods,
>>> we can end up spending significant time in
>>> DefaultMethods::generate_default_methods
>>>
>>> This optimization reduces work done and memory requirements by reusing
>>> allocated data structures. For example by maintaining free lists of
>>> allocated Node objects.
>>>
>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8233497
>>> Webrev: http://cr.openjdk.java.net/~redestad/8233497/open.00/
>>>
>>> Testing: Tier1-3, will make sure tier4-7 pass before push
>>>
>>> Performance notes: On one of our more complex startup tests we see a 3%
>>> improvement on the execution time total. Much less on simpler
>>> applications.
>>>
>>> I've not done a formal complexity analysis, but I think the memory
>>> complexity is now down from O(N*M) to O(N+M) where N is the number of
>>> classes and interfaces in the hierarchy and M the number of methods of
>>> interest in that hierarchy. Algorithmic complexity is probably O(N*M)
>>> still, but with much better constants.
>>>
>>> Special thanks to Lois for patience and persistence over several rounds
>>> of pre-review!
>>>
>>> Thanks!
>>>
>>> /Claes
>>
>> Hi Claes,
>>
>> I have reviewed your final webrev and I think this looks good. It 
>> certainly is an area that can benefit from performance improvements 
>> so thank you for putting the time in on this. 
>
> Lois, again thank you for reviewing and suggesting a lot of improvements
> along the way!
>
>> A couple of final comments:
>>
>> - line #79: Can you update the comment, it indicates that 
>> new_node_data takes an InstanceKlass* parameter which has been removed
>>
>> - line #716: I would like an assert as part of the if statement in 
>> the situation where _free_scopes is not empty and the 
>> StateRestoreScope node is being popped instead of newly allocated. 
>> The assert would then check if that StateRestorerScope's _marks 
>> GrowableArray is empty, which it should be.  That would make me more 
>> comfortable with the idea that a previous StateRestoreScope's _marks 
>> array's data is not getting mixed with new data being established for 
>> a new Node in the hierarchy.
>>
>
> Done:
>
> http://cr.openjdk.java.net/~redestad/8233497/open.01/

Thanks for making that change!  Please fix comment at line #79 and add 
some comment to the assert at line #721 like "StateRestorerScope's 
_marks array not empty"?

I don't need to see another webrev.

Lois

>
> /Claes