Bug 118573 – Optimize the thread locks for API Shims

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yi Shen]

Remove the thread lock from API Shims if the VM is not a APIShared type. The similar logic could be found in old revision of JSLock, which does the lock or unlock only when exec->globalData().isSharedInstance() is true. By removing the thread lock, the JSC API static function call could be 60% times faster than before.

[Yi Shen]

Created attachment 206479 [details]
Test case

[Yi Shen]

                 No cache        Cache
result(ms)
1.                  8000          3190
2.                  7600          3160
3.                  7987          3208
4.                  8002          3305
5.                  8155          3173
6.                  7539          3125
7.                  8562          3135
8.                  7455          3137
9.                  7936          3122
10.                 7461          3103
---------------------------------------
avg                 7869          3166

[Yi Shen]

The first row should be     
             With lock          Without lock
(In reply to comment #2)
>                  No cache        Cache
> result(ms)
> 1.                  8000          3190
> 2.                  7600          3160
> 3.                  7987          3208
> 4.                  8002          3305
> 5.                  8155          3173
> 6.                  7539          3125
> 7.                  8562          3135
> 8.                  7455          3137
> 9.                  7936          3122
> 10.                 7461          3103
> ---------------------------------------
> avg                 7869          3166

[Yi Shen]

Created attachment 206480 [details]
proposal patch

[Geoffrey Garen]

Comment on attachment 206480 [details]
proposal patch

Even unshared VMs need locking, since they can be used on more than one thread.

I'm intrigued by your test case, though. Maybe there's another way to optimize this.

[Chang Shu]

(In reply to comment #5)
> (From update of attachment 206480 [details])
> Even unshared VMs need locking, since they can be used on more than one thread.
> 
> I'm intrigued by your test case, though. Maybe there's another way to optimize this.

I opened a bug with the same test before and Yi is trying to resolve this issue through a series of patches. Your help is very appreciated.

https://bugs.webkit.org/show_bug.cgi?id=97122

[Yi Shen]

Thanks for review. How about removing the API thread lock/unlock for the VM which has an exclusive thread? For example, WK2 is using JSC APIs within the injected bundle, which runs in the WebCore's exclusive VM. I believe it is thread safety when executing JSC APIs in the WebCore's VM.  

(In reply to comment #5)
> (From update of attachment 206480 [details])
> Even unshared VMs need locking, since they can be used on more than one thread.
> 
> I'm intrigued by your test case, though. Maybe there's another way to optimize this.

[Geoffrey Garen]

> Thanks for review. How about removing the API thread lock/unlock for the VM which has an exclusive thread? For example, WK2 is using JSC APIs within the injected bundle, which runs in the WebCore's exclusive VM. I believe it is thread safety when executing JSC APIs in the WebCore's VM.  

I think a specialized design like that would need a more real-world benchmark to justify itself.

There are lots of opportunities for optimizing JSLock.

[Yi Shen]

I do have a more real-world test for this. Will add a patch for review and post a real-world benchmark next week. Thanks in advanced!
(In reply to comment #8)
> > Thanks for review. How about removing the API thread lock/unlock for the VM which has an exclusive thread? For example, WK2 is using JSC APIs within the injected bundle, which runs in the WebCore's exclusive VM. I believe it is thread safety when executing JSC APIs in the WebCore's VM.  
> 
> I think a specialized design like that would need a more real-world benchmark to justify itself.
> 
> There are lots of opportunities for optimizing JSLock.

[Yi Shen]

Created attachment 206576 [details]
Remove the thread lock from API Shims if the VM has an exclusive thread

[Yi Shen]

Created attachment 207361 [details]
Test sample code for JSC API

[Yi Shen]

Created attachment 207363 [details]
Test result

[Yi Shen]

The added test code is porting from JSTweener by using JSC APIs (WebKit2 injected bundle). http://coderepos.org/share/wiki/JSTweener

The test is a rendering heavy page, so the PFS changes for tuning on/off thread lock may not be 'significant' - the average fps was increased from ~31 to ~34 on my machine after applied my patch. (I was hoping I can add a JS property accessing heavy page, but which turns out it needs a lot more work) In addition, I have run this test on my Mac for hundreds times, and no thread crash observed.

[Geoffrey Garen]

Comment on attachment 206576 [details]
Remove the thread lock from API Shims if the VM has an exclusive thread

r=me

[Yi Shen]

Thanks for reviewing it :)
(In reply to comment #14)
> (From update of attachment 206576 [details])
> r=me

[WebKit Commit Bot]

Comment on attachment 206576 [details]
Remove the thread lock from API Shims if the VM has an exclusive thread

Clearing flags on attachment: 206576

Committed r153331: <http://trac.webkit.org/changeset/153331>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.