RR(S): 8009062 poor performance of JNI AttachCurrentThread after fix for 7017193

Wed May 8 19:00:01 PDT 2013

Hi Dmitry,

On 8/05/2013 11:27 PM, Dmitry Samersoff wrote:
> David,
>
> Thank you for detailed review:
>
> Updated webrev is here:
>
> http://cr.openjdk.java.net/~dsamersoff/8009062/webrev.05/
>
> 1. Formatting nits fixed (hopefully)
> 2. More comments added
> 3. Debug code is placed under #ifndef PRODUCT and cleaned up.
>     I plan to remove it somewhere in the future when my
>     changes bakes for some time in nightlies.

I haven't re-examined this yet.

>> 1124 // Locate initial thread stack. This special handling of initial
>> thread stack
>> 1125 // is needed because pthread_getattr_np() on most (all?) Linux
>> distros returns
>> 1126 // bogus value for initial thread.
>>
>> ??? So how is this working?
>
> I suspect this code is not updated for a long time. e.g. modern hotspot
> will not work at all if /proc is not mounted and statement about bogus
> pthread_getattr_np results is not true since NPTL (i.e. 2.6 kernel)

The manpage for pthread_getattr_np still states:

"In addition, if thread refers to the main thread, then 
pthread_getattr_np() can fail because of errors from various underlying 
calls: fopen(3), if /proc/self/maps can't be opened; and getrlimit(2), 
if the RLIMIT_STACK resource limit is not supported. "

so it is far from clear to me that this main thread problem no longer 
applies on 2.6 systems. The hotspot code essentially does the same as 
the pthread_getattr_np implementation but has various fallbacks if it 
encounters the above errors (they do not cause the VM to abort AFAICS).

So I don't think you can just use pthread_getattr_np for the main thread 
without having a way to handle failure. That said we already jump 
through all these hoops in os::Linux::capture_initial_stack so why 
aren't we simply using the values of

  _initial_thread_stack_size
  _initial_thread_stack_bottom

that we calculated at startup?

Thanks,
David
-----

> Since we don't support 2.4 kernel anymore lots of code could be
> simplified drastically.
>
> Do you want me to cleanup this code as well?
>
> -Dmitry
>
>
> On 2013-05-08 05:18, David Holmes wrote:
>> Hi Dmitry,
>>
>> First a few minor cleanups:
>>
>> - use spaces around all operators ie imax = imid; not imax=imid; etc
>> - don't use %p use appropriate *FORMAT macro
>> - don't leave commented out tty->print (either delete or guard with a
>> trace/debug flag or Verbose flag)
>>
>> ---
>>
>> os_linux.cpp:
>>
>> I'm not sure about the "#ifdef DEBUG_STACK_GUARD_PAGES". Do we need to
>> keep this code? If so should it just be non-product?
>>
>> I don't understand this guarantee:
>>
>> 2930     } else{
>> 2931            guarantee(res == false, "growable stack in non-initial
>> thread");
>> 2932     }
>>
>> What in the previous code indicates this is a growable stack?
>>
>>
>> 2961   bool chk_bounds = os::Linux::is_initial_thread();
>> 2962   if (chk_bounds) {
>>
>> We don't need the chk_bounds local any more.
>>
>> If I'm reading things right get_stack_bounds is only called for the
>> initial thread, but you then call pthread_getattr_np  but that isn't
>> valid as stated elsewhere:
>>
>> 1124 // Locate initial thread stack. This special handling of initial
>> thread stack
>> 1125 // is needed because pthread_getattr_np() on most (all?) Linux
>> distros returns
>> 1126 // bogus value for initial thread.
>>
>> ??? So how is this working?
>>
>> Also you should save the return values of the pthread_* functions and
>> use assert_state to check them for errors. (Yes I know this is handled
>> very inconsistently through the code but we should be moving towards
>> full error checking.)
>>
>> Overall I'm unclear about the detailed logic being applied here (in both
>> old code and new!).
>>
>> Thanks,
>> David
>> -----
>>
>> On 23/04/2013 7:41 AM, Dmitry Samersoff wrote:
>>> Hi Everybody,
>>>
>>> Here is webrev of proposed changes:
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/8009062/webrev.04/
>>>
>>> Any comments is much appreciated.
>>>
>>> The problem:
>>>
>>> Under Linux stack of main thread is growable, so we have to make sure
>>> that address we plan to put a guard pages to and below is not mapped.
>>>
>>> Historically we find bounds of the stack of main thread by seeking
>>> /proc/self/maps for "[stack]" and parsing this line.
>>>
>>> I don't like buffered reading of /proc files and sometimes ago rewrite
>>> this function to read it byte-to-byte. Unfortunately, resulting
>>> performance penalties is not acceptable.
>>>
>>>
>>> Solution:
>>>
>>> Below is slightly different approach - I use mincore(2) to check whether
>>> the page is mapped or not. Typically mincore(2) is used to check whether
>>> the page is resides in physical memory or not, but this function returns
>>> -1 and set errno to ENOMEM if a region we are asking about contains not
>>> mapped memory.
>>>
>>> Testing:
>>>
>>> Passed jprt and couple of jtreg tests. No special regression test
>>> necessary as the test for 6929067 covers this case as well.
>>>
>>> -Dmitry
>>>
>>>
>>>
>>> On 2013-03-28 23:02, Adam Domurad wrote:
>>>> On 03/11/2013 01:19 PM, Dmitry Samersoff wrote:
>>>>> Adam,
>>>>>
>>>>> Sorry! I can't provide any estimations right now. But it's a high
>>>>> priority issue in my queue, and I plan to provide a fix as soon as it
>>>>> become possible.
>>>>>
>>>>> -Dmitry
>>>>>
>>>>> On 2013-03-06 21:45, Adam Domurad wrote:
>>>>>> On 03/01/2013 11:00 AM, Dmitry Samersoff wrote:
>>>>>>> Adam,
>>>>>>>
>>>>>>> Thank you for advice! I'm looking for a possibility to remove
>>>>>>> get_stack_bounds function entirely as Dean suggested. But it takes
>>>>>>> some
>>>>>>> time.
>>>>>> Hi again, not to be a bother, but do you have a rough time-frame that
>>>>>> the fix will take?
>>>>>>
>>>>>> Thanks,
>>>>>> -Adam
>>>>>
>>>>
>>>> As per our off-list chat, I have attached my implementation of a revised
>>>> implementation of get_stack_bounds with a small test-case. As far as
>>>> performance is concerned, it's an order of magnitude faster from my
>>>> timings. This should be suitable given that no problems were reported
>>>> with the old implementation.
>>>>
>>>> Happy hacking,
>>>> -Adam
>>>
>>>
>
>