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

[Dmitry Samersoff]

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.


> 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)

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
>>
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer