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

[Calvin Cheung]

Thanks again - David, Harold.

Calvin

On 6/6/16, 10:15 AM, harold seigel wrote:
> Hi Calvin,
>
> It looks good to me, also.
>
> Thanks, Harold
>
>
> On 6/5/2016 9:19 PM, David Holmes wrote:
>> On 4/06/2016 9:10 AM, Calvin Cheung wrote:
>>> Hi Harold, David,
>>>
>>> I've updated the webrev with ResourceMark and reverted the changes in
>>> metaspaceShared.cpp.
>>> http://cr.openjdk.java.net/~ccheung/8153876/webrev.01/
>>
>> Looks good.
>>
>> Thanks,
>> David
>>
>>>
>>> On 6/3/16, 12:06 PM, 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.
>>> Except for ClassPathDirEntry::open_stream(). The buffer will be 
>>> added to
>>> the ClassFileStream but the path is a local variable. So I've added
>>> FREE_RESOURCE_ARRAY for path in several places.
>>>>
>>>> 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);.
>>> Yes, they've been added in the updated webrev.
>>>
>>> I'll rerun some tests.
>>>
>>> thanks,
>>> Calvin
>>>
>>>>
>>>> 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
>>>>
>