Bug 35991

Summary: Would be faster to not use a thread specific to implement StringImpl::empty()
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: PlatformAssignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
The patch
ggaren: review-
The patch mjs: review+

Gavin Barraclough
Reported 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).
Attachments
The patch (15.74 KB, patch)
2010-03-10 15:34 PST, Gavin Barraclough 		ggaren: review-
DetailsFormatted DiffDiff
The patch (16.74 KB, patch)
2010-03-10 17:21 PST, Gavin Barraclough 		mjs: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Gavin Barraclough
Comment 1 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.
Darin Adler
Comment 2 2010-03-10 15:50:26 PST
Comment on attachment 50447 [details] The patch r=me assuming you ran all the regression tests
Geoffrey Garen
Comment 3 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.
Darin Adler
Comment 4 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.
Gavin Barraclough
Comment 5 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.
Gavin Barraclough
Comment 6 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?
Maciej Stachowiak
Comment 7 2010-03-10 18:11:32 PST
Comment on attachment 50456 [details] The patch r=me
Gavin Barraclough
Comment 8 2010-03-10 19:53:22 PST
landed in r55825
Note You need to log in before you can comment on or make changes to this bug.