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

[David Holmes]

On 4/06/2016 5:06 AM, harold seigel wrote:
> Hi Calvin,
>
> Methods that allocate memory using NEW_RESOURCE_ARRAY and do not have
> ResourceMarks probably want to return the allocated memory to their
> caller.  I think that most of the places where you added
> NEW_RESOURCE_ARRAY allocations also should have ResourceMarks.

Right. Either a RM or an explicit free is neededin general. In the case 
of Arguments::build_resource_string() it is an example of resources 
allocated in the main thread that don't get freed until the VM terminates.

Will look at updated webrev on Monday.

Thanks,
David

> ResourceMarks should be added to classLoader.cpp at lines 346, 511, and
> 753.  If THREAD is available then please add the ResourceMarks as
> ResourceMark rm(THREAD);.
>
> Thanks, Harold
>
>
> On 6/3/2016 2:06 PM, Calvin Cheung wrote:
>> 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
>