Bug 35991 – Would be faster to not use a thread specific to implement StringImpl::empty()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Gavin Barraclough]

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).

[Gavin Barraclough]

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 on attachment 50447 [details]
The patch

r=me assuming you ran all the regression tests

[Geoffrey Garen]

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]

(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]

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]

Implemented fix as discussed with geoff on IRC, does anyone know a better startup hook I should be using in WebCore?

[Maciej Stachowiak]

Comment on attachment 50456 [details]
The patch

r=me

[Gavin Barraclough]

landed in r55825