426585 - Additional cache XPCPerThreadData for mainthread

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[gyuyoung kim]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ko; rv:1.8.1.13) Gecko/20080311 Firefox/2.0.0.13
Build Identifier: 

As mentioned in Bug 412698 comment 13, I made a new bug about it.

Reproducible: Always

Comment 1

[gyuyoung kim]

Attachment: Additional patch

Comment 2

[gyuyoung kim]

I tested the patch in order to know how many we can get a performance gain from it.

I measured the execution time of GetData() until firefox displays the front page of three websites (youtube, espn, newyork times) all together from starting the firefox.

I tested 10 times with each cases. 
The result is as below, 
  * Current source code:
      -	Average total called count of GetData() : 80,744 times
      -	Average total time of GetData() : 24,317 microsec.
      -	Used time per 1 calling : About 0.301152358 microsec.
   
  * The patch is adjusted:
      -	Average total called count of GetData() : 78,128 times
      -	Average total time of GetData() : 23,415 microsec.
      -	Used time per 1 calling : About 0.299680646 microsec.
   
Of course, my test result can has an error. But, as mentioned in Mr. Stenback, there is not a big performance gain. On the contrary, a performance gain is too small.

It seems to me that my patch can get a little of performance gain when firefox is run during long time or with many tab browsing.

So, it seems that I can't insist the patch's approval.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 313148 [details] [diff] [review]
Additional patch

>Index: xpcprivate.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v
>retrieving revision 1.280
>diff -u -8 -p -r1.280 xpcprivate.h
>--- xpcprivate.h	29 Feb 2008 02:09:10 -0000	1.280
>+++ xpcprivate.h	1 Apr 2008 13:58:25 -0000
>@@ -3087,16 +3087,20 @@ public:
>     {
>         if(cx)
>         {
>             NS_ASSERTION(cx->thread, "Uh, JS context w/o a thread?");
> 
>             if(cx->thread == sMainJSThread)
>                 return sMainThreadData;
>         }
>+        else{

Style nit: brace goe on next line, indented to underhang the first 'e' in 'else'.

>+            if(NS_IsMainThread() && sMainThreadData)

Another nit: other xpconnect code uses 'else if ' so you could avoid the braces around the else clause and reduce the indentation of the return here.

Non-nit: NS_IsMainThread() seems slower than PR_GetCurrentThread() == sMainThreadData->mThread.

>+                return sMainThreadData;
>+        }

See if you get a better win with the above non-nit, and please post updated performance results. Thanks,

/be

Comment 4

[gyuyoung kim]

Attachment: patch with PR_GetCurrentThread()

I tested the patch with PR_GetCurrentThread().

As a conclusion, the patch with Brendan's comment had the best result. But, the performance gain is small. 

As I tested it before, I measured the execution time of GetData() until firefox displays the front page of three websites (ESPN, NewYork Times, CNN) simultaneously from starting the firefox.
And, I tested 10 times on each cases,
(I tried to profile them exactly, but the result can have a little error.)

Test results are as below,
      -	Current source
            Average called count of GetData() : 88,685.2 times
            Average execution time of GetData() : 26,672 microsec
            Average time per 1 execution of GetData() : 0.301152358 microsec

      -	New patch with PR_GetCurrentThread()
            Average called count of GetData() : 84,427.9 times
            Average execution time of GetData() : 24,730.4 microsec 
            Average time per 1 execution of GetData() : 0.292934667 microsec

System Under Test
      -	CPU : Intel ® Core 2 Duo 6400 2.13 Ghz
      -	RAM : 2GB
      -	OS: Linux 
      -	Kernel Info: 2.6.18-6-686
      -	Time measurement: I used the “struct timeval”

Comment 5

[gyuyoung kim]

Attachment: v2-patch with PR_GetCurrentThread()

The previous test has a big problem. The problem is that it seems mThread of XPCPerThreadData class doesn't be initialized. So, sMainThreadData->mThread == PR_GetCurrentThread() couldn't be a true. In other words, an additional caching couldn’t be adjusted in GetData(). It is my mistake.

I modified the GetDataImpl() in order to use the sMainThreadData->mThread, as below,
    if(cx && !sMainJSThread && NS_IsMainThread())
    {
        sMainJSThread = cx->thread;
       
        sMainThreadData = data;
	
+	sMainThreadData->mThread = PR_GetCurrentThread();
    }

I tested them again with a modified source. The test scenario is same as I tested them before. The test results are as below,
      - Current source
            Average called count of GetData() : 84,037.9 times
            Average execution time of GetData() : 24,563.5 usec
            Average time per 1 execution of GetData() : 0.292247122 usec
	    Average called count of GetDataImpl() : 13,190.5 times 	
	
      - New patch with PR_GetCurrentThread()
            Average called count of GetData() : 84,455.7 times
            Average execution time of GetData() : 23,480 microsec 
            Average time per 1 execution of GetData() : 0.278013925 microsec
. 	    Average called count of GetDataImpl() : 337.8 times
            Average called count of additional caching : 13,284 times

As a conclusion, the patch with PR_GetCurrentThread() has the best result.

System Under Test
      - CPU : Intel ® Core 2 Duo 6400 2.13 Ghz
      - RAM : 2GB
      - OS: Linux 
      - Kernel Info: 2.6.18-6-686
      - Time measurement: I used the “struct timeval”

BTW, it seems to me that a role of mThread in XPCPerThreadData class is to store the reference of thread which uses the XPCPerThreadData instance of mThread. If it is so, in my opinion, mThread variable need to be initialized in XPCPerThreadData() constructor as below. 
    
     XPCPerThreadData::XPCPerThreadData()
     :   …
+    mThread(nsnull),
       …
     {
        if(gLock)
       {
        nsAutoLock lock(gLock);
        mNextThread = gThreads;
        gThreads = this;
      }
+     mThread=PR_GetCurrentThread();
    }

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

My crazy idea:
- note the start address of the stack for each thread as we create them
- sort them according to the platform's stack-growth direction
- look at an address of a local to see if we're in the main thread's range

A bit of book-keeping, but it would make "isMainThread" extremely cheap...

Comment 7

[gyuyoung kim]

It seems to me we should modify the GetData() considerably in order to adjust an idea of Mr.Shaver, right?
And,if we can use isMainthread variable,I think it is better than using other skill s for performance.
Brendan, How do you think about it?

Comment 8

[Brendan Eich [:brendan]]

Still waiting on dbradley for r+, or a new patch per shaver's idea (which is clever and sounds like work ;-). I'm ok with making progress using the current patch if it passes David's review. The shaver-ific idea should go in a followup bug in that case.

/be

Comment 9

[gyuyoung kim]

As Brendan said, I'd like to know Mr. bradley's thought about the patch as well.
Please give your comments or review to me.

Comment 10

[gyuyoung kim]

It seems to me Mr.bradley is busy. If he is so,I'd like to change the reviewer. Please,let me know who can review the patch.

Comment 11

[David Bradley]

Comment on attachment 314127 [details] [diff] [review]
v2-patch with PR_GetCurrentThread()

r=dbradley

Nit, indent the else if to match the block.

What I found interesting is that before this patch, XPCPerThreadData::mThread doesn't appear to be used at all. Timeless moved it out of XPC_CHECK_WRAPPER_THREADSAFETY. But as far as I can tell wasn't used.

We can go Shaver's route later if someone gets ambitious and provides a patch, but I think this is fine for now.

Comment 12

[David Bradley]

Also I don't see comments on the bug unless I'm cc'd ;-)

Comment 13

[gyuyoung kim]

When I made the patch with the mThread variable of XPCPerThreadData, I also couldn't find where the mThread was used as Mr. Bradley said above. I think this patch can be adjust temporally until go Shaver's route later.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 314127 [details] [diff] [review]
v2-patch with PR_GetCurrentThread()

>+	else if(sMainThreadData && (sMainThreadData->mThread == PR_GetCurrentThread()))
>+        {
>+	    return sMainThreadData;
>+        }

Indentation is off (by two few spaces)  on the else and return lines that were added. Hope this is not due to hard tabs -- please expand all such, per the Emacs modeline at the top of the file.

/be

Comment 15

[gyuyoung kim]

Attachment: v3-patch with PR_GetCurrentThread()

I modified my patch with Brendan and Mr.Bradley's advice. If the modified patch still has any nits, please let me know. Thank you.

Comment 16

[David Bradley]

Comment on attachment 322033 [details] [diff] [review]
v3-patch with PR_GetCurrentThread()

r=dbradley

Looks fine.

For future reference you don't really need to repost for review for minor issues.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 322033 [details] [diff] [review]
v3-patch with PR_GetCurrentThread()

>+        else if(sMainThreadData && (sMainThreadData->mThread == PR_GetCurrentThread()))

Final nit, no need to re-request reviews -- parenthesizing == against && is unnecessary and prevailing style in xpconnect does not so over-parenthesize. If you put up a patch to land without the parentheses around the == expression, just mark it r+ and sr+ and add the checkin-needed keyword to the bug. This can land in the appropriate post-1.9 repository. Thanks,

/be

Comment 18

[gyuyoung kim]

Attachment: v4-patch with PR_GetCurrentThread() for checkin

I attach new patch without the over-parenthesize.
Thanks.

Comment 19

[gyuyoung kim]

I am waiting to merge this patch into mozilla trunk in order to change the status of bug with the FIXED.

Comment 20

[Reed Loden [:reed]]

Pushed in 15864:cb22a612ced5.