RFR(xs): 8153876: Replace 4K stack allocations with Resource allocations

[Calvin Cheung]

Hi David,

Thanks for your review.

On 6/2/16, 6:05 PM, David Holmes wrote:
> Hi Calvin,
>
> On 3/06/2016 9:13 AM, Calvin Cheung wrote:
>>
>> Please review this small fix for replacing some of the array
>> declarations with the resource allocation macros.
>
> If you add a resource allocation don't you also need to ensure there 
> is a ResourceMark to clean it up again?
I looked at some of the existing usage and none of them has a ResourceMark.
One example is in Arguments::build_resource_string().

>
> And what is the overhead of using resource allocation versus stack 
> allocation? A lot of this seems to be in startup code.
Since my fix touches the ctw_visitor() in classLoader.cpp, I tried to 
use the -XX:+CompileTheWorld to see if there's any difference in perf 
before and after the fix.

I modified the function so that it only ran a limited number of 
iterations. For a full run, it took more than 7 hrs.:
CompileTheWorld : Done (41453 classes, 518964 methods, 25723180 ms)

100 iterations
------------
no fix
======
CompileTheWorld : Done (103 classes, 2096 methods, 66605 ms)
CompileTheWorld : Done (103 classes, 2096 methods, 67171 ms)
CompileTheWorld : Done (103 classes, 2096 methods, 66376 ms)

with fix
========
CompileTheWorld : Done (103 classes, 2096 methods, 66694 ms)
CompileTheWorld : Done (103 classes, 2096 methods, 66358 ms)
CompileTheWorld : Done (103 classes, 2096 methods, 66622 ms)



300 iterations
------------
no fix
------
CompileTheWorld : Done (321 classes, 4484 methods, 145322 ms)
CompileTheWorld : Done (321 classes, 4484 methods, 144472 ms)
CompileTheWorld : Done (321 classes, 4484 methods, 144229 ms)

with fix
--------
CompileTheWorld : Done (321 classes, 4484 methods, 144748 ms)
CompileTheWorld : Done (321 classes, 4484 methods, 145070 ms)
CompileTheWorld : Done (321 classes, 4484 methods, 144773 ms)

So the times are pretty much the same with and without the fix.

Below is the disassembly of the NEW_RESOURCE_ARRAY:
  511                  char* path = NEW_RESOURCE_ARRAY(char, 
JIMAGE_MAX_PATH);
00007ffff6736f7d:   mov $0x1000,%eax
00007ffff6736f82:   mov $0x0,%esi
00007ffff6736f87:   mov %rax,%rdi
00007ffff6736f8a:   callq 0x7ffff7079ca0 
<_Z23resource_allocate_bytesmN17AllocFailStrategy13AllocFailEnumE>
00007ffff6736f8f:   mov %rax,-0x10(%rbp)
>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8153876
>> webrev: http://cr.openjdk.java.net/~ccheung/8153876/webrev.00/
>
> src/share/vm/memory/metaspaceShared.cpp
>
> This change seems unnecessary - we don't have stackoverflow concerns 
> when doing a dump.
I was thinking the same. I will revert the change.

thanks,
Calvin
>
> Thanks,
> David
>
>> Testing:
>>     JPRT
>>     non-colo "quick" tests
>>
>> thanks,
>> Calvin