Bug 35991 - Would be faster to not use a thread specific to implement StringImpl::empty()
Summary: Would be faster to not use a thread specific to implement StringImpl::empty()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-10 15:29 PST by Gavin Barraclough
Modified: 2010-03-10 19:53 PST (History)
1 user (show)

See Also:


Attachments
The patch (15.74 KB, patch)
2010-03-10 15:34 PST, Gavin Barraclough
ggaren: review-
Details | Formatted Diff | Diff
The patch (16.74 KB, patch)
2010-03-10 17:21 PST, Gavin Barraclough
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-03-10 15:29:42 PST
Copy JavaScriptCore in making 'static' strings threadsafe, make the empty string a static, shared by all threads.

Change JSC::UStringImpl's implementation to match (use a static defined within the empty() method), change interface to match StringImpl (return a pointer not a reference).
Comment 1 Gavin Barraclough 2010-03-10 15:34:54 PST
Created attachment 50447 [details]
The patch

 ~0% performance impact in JSC (possible minor progression from moving empty() from .h to .cpp).
~2% progression on Dromaeo DOM core & JS lib tests.
Comment 2 Darin Adler 2010-03-10 15:50:26 PST
Comment on attachment 50447 [details]
The patch

r=me assuming you ran all the regression tests
Comment 3 Geoffrey Garen 2010-03-10 16:27:50 PST
Comment on attachment 50447 [details]
The patch

Sorry -- I said r- on IRC because DEFINE_STATIC_LOCAL is not threadsafe. The solution we agreed upon was to add a call to empty() at startup, to ensure that initialization of the shared empty static occurs before any JSC or WebCore threads are created.
Comment 4 Darin Adler 2010-03-10 16:32:52 PST
(In reply to comment #3)
> Sorry -- I said r- on IRC because DEFINE_STATIC_LOCAL is not threadsafe. The
> solution we agreed upon was to add a call to empty() at startup, to ensure that
> initialization of the shared empty static occurs before any JSC or WebCore
> threads are created.

WebCore has “startup” but I’m not sure JavaScriptCore does.
Comment 5 Gavin Barraclough 2010-03-10 17:21:44 PST
Created attachment 50456 [details]
The patch

Added call to UStringImpl::empty() from initializeThreadingOnce() to ensure empty() is called in a threadsafe fashion the first time it is used.
Added call to StringImpl::empty() from the ThreadGlobalData constructor, to ensure empty() has been called from the main thread before any further threads are created.
Comment 6 Gavin Barraclough 2010-03-10 17:22:33 PST
Implemented fix as discussed with geoff on IRC, does anyone know a better startup hook I should be using in WebCore?
Comment 7 Maciej Stachowiak 2010-03-10 18:11:32 PST
Comment on attachment 50456 [details]
The patch

r=me
Comment 8 Gavin Barraclough 2010-03-10 19:53:22 PST
landed in r55825