Review request for 6977034 Thread.getState() very slow

[David Holmes]

Thumbs up from me.

Thanks,
David

Mandy Chung said the following on 12/07/10 05:26:
>   Remi, Eamonn, Brian, David, Doug,
> 
> Thanks for the feedback.
> 
> On 12/04/10 04:22, Eamonn McManus wrote:
>> Hi Mandy,
>>
>> This test:
>>
>>         if ((threadStatus & JVMTI_THREAD_STATE_RUNNABLE) == 1) {
>>
>> is always false, since JVMTI_THREAD_STATE_RUNNABLE is 4. (NetBeans 7.0 
>> helpfully flags this; I'm not sure if earlier versions do.)
>>
> 
> Good catch.   This explains why the speed up for RUNNABLE was not as 
> high in the microbenchmark measurement.  Correcting it shows that 
> Thread.getState() gets 3.5X speed up on a thread in RUNNABLE state.
> 
>> But, once corrected, I think you could use this idea further to write 
>> a much simpler and faster method, on these lines:
>>
>>     public static Thread.State toThreadState(int threadStatus) {
>>         if ((threadStatus & JVMTI_THREAD_STATE_RUNNABLE) *!= 0*) {
>>             return RUNNABLE;
>>         } else if ((threadStatus & JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER) != 0) {
>>             return BLOCKED;
>>         } else if ((threadStatus & JVMTI_THREAD_STATE_WAITING_WITH_TIMEOUT) != 0) {
>>             return TIMED_WAITING;
>>         } else if ((threadStatus & JVMTI_THREAD_STATE_WAITING_INDEFINITELY) != 0) {
>>             return WAITING;
>>         } else if ((threadStatus & JVMTI_THREAD_STATE_TERMINATED) != 0) {
>>             return TERMINATED;
>>         } else {
>>             return NEW;
>>         }
>>     }
> 
> I forgot to mention in the email that I implemented this simpler 
> approach to compare with the table lookup approach.   There were no 
> significant difference.  I now rerun with the corrected fix (checking != 
> 0 rather than == 1) and the table lookup approach is about 2-6% faster 
> than the sequence of tests approach.
> 
> I am also for the simpler approach but I post the table lookup approach 
> as a proposed fix to get any opinion on the performance aspect with that 
> approach.
> 
> Given that the Fork-Join framework doesn't depend on it, I will go for a 
> simpler approach (sequence of tests) and further tune its performance 
> when there is a use case requiring a perf improvement.
> 
> New webrev:
>    http://cr.openjdk.java.net/~mchung/6977034/webrev.01/
> 
> Can you review this version?
> 
> Thanks
> Mandy
> 
>> You could tweak the order of the tests based on what might be the 
>> relative frequency of the different states but it probably isn't worth it.
>>
>> Regards,
>> Éamonn
>>
>> On 3/12/10 11:52 PM, Mandy Chung wrote:
>>> Fix for 6977034: Thread.getState() very slow
>>>
>>> Webrev at:
>>>    http://cr.openjdk.java.net/~mchung/6977034/webrev.00/
>>>
>>> This is an improvement to map a Thread's threadStatus field to 
>>> Thread.State.  The VM updates the Thread.threadStatus field directly 
>>> at state transition with the value as defined in JVM TI [1].  The 
>>> java.lang.Thread.getState() implementation can directly access the 
>>> threadStatus value and do a direct lookup from an array of 
>>> Thread.State.  The threadStatus value is a bit vector and we would 
>>> have to create an array of a minimum of 1061 (0x425) elements to do 
>>> direct mapping.   I took the approach to use the first highest order 
>>> bit set to 1 in the masked threadStatus value as the index to the 
>>> Thread.State element and only caches 32 elements (could be fewer).  I 
>>> wrote a micro-benchmark measuring the Thread.getState of a thread in 
>>> different state that shows 1.7X to 6X speedup (see below).  There is 
>>> possibly some issue with my micro-benchmark that I didn't observe the 
>>> 14X speed up as Doug did in his experiment.  However, I'd like to get 
>>> this reviewed and pushed to the repository so that anyone can do more 
>>> experiment on the performance measurement.
>>>
>>> Thanks
>>> Mandy
>>> P.S. The discussion on this thread can be found at [2] [3].
>>>
>>> [1] 
>>> http://download.java.net/jdk7/docs/platform/jvmti/jvmti.html#GetThreadState
>>> [2] 
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-July/004567.html
>>> [3] 
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-August/004721.html
>>>
>>>
>>> 	JDK 7 b120 (in ms)	With fix (in ms)	Speed up
>>> main		46465	        22772			2.04
>>> NEW		50676		29921			1.69
>>> RUNNABLE	42202		14690			2.87
>>> BLOCKED		72773		12296			5.92
>>> WAITING		48811		13041			3.74
>>> TIMED_WAITING	45737		12849			3.56
>>> TERMINATED	40314		16376			2.46
>>>
>