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

[David Holmes]

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