Refactoring LLInt::Data to be a singleton

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Refactoring LLInt::Data to be a singleton.  There was no reason why it shouldn't be a singleton in the first place anyway.  The previous form was being used as a singleton, but instantiates LLInt::Data like an instance object.  It is now made up of only static members.

This change allows its opcodeMap to be easily queried from any function without needing to go through a GlobalData object.  It also introduces the LLInt::getCodePtr() methods that will be used by the LLInt C loop later to redefine how llint symbols (opcodes and trampoline glue labels) get resolved.

[Mark Lam]

Created attachment 161189 [details]
Fix.

[Gyuyoung Kim]

Comment on attachment 161189 [details]
Fix.

Attachment 161189 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13687197

[Early Warning System Bot]

Comment on attachment 161189 [details]
Fix.

Attachment 161189 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13688184

[Early Warning System Bot]

Comment on attachment 161189 [details]
Fix.

Attachment 161189 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13686170

[Mark Lam]

Created attachment 161195 [details]
Fixed typo.

[Geoffrey Garen]

Comment on attachment 161195 [details]
Fixed typo.

View in context: https://bugs.webkit.org/attachment.cgi?id=161195&action=review

Looks good to me. I have one edit I'd like to see before marking this r+.

> Source/JavaScriptCore/llint/LLIntData.cpp:44
> +    if (!Data::s_isInitialized) {

Please remove s_isInitialized and this test of it. This test is neither necessary nor sufficient for a thread-safe singleton. It isn't necessary because you're using the initializeThreading() hook to ensure a thread-safe singleton. It isn't sufficient because this branch isn't guarded by a lock.

Side note about WebKit style: When we do use checks like this, we prefer "if (!condition) { early return }" style, to reduce the amount of indentation in a function.

[Mark Lam]

Created attachment 161237 [details]
Removed s_isInitialized and its references.

[Geoffrey Garen]

Comment on attachment 161237 [details]
Removed s_isInitialized and its references.

r=me

[WebKit Review Bot]

Comment on attachment 161237 [details]
Removed s_isInitialized and its references.

Attachment 161237 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13695333

New failing tests:
CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread

[Geoffrey Garen]

Comment on attachment 161237 [details]
Removed s_isInitialized and its references.

cq+ again because this patch did not break the cr-linux bot, it was already broken.

[WebKit Review Bot]

Comment on attachment 161237 [details]
Removed s_isInitialized and its references.

Rejecting attachment 161237 [details] from commit-queue.

New failing tests:
CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread
Full output: http://queues.webkit.org/results/13685512

[Gavin Barraclough]

Landed in r127068.

[Gavin Barraclough]

This is a JSC change, so the EWS failure on chromium must be a flake.