Bug 95316 - Refactoring LLInt::Data to be a singleton
Summary: Refactoring LLInt::Data to be a singleton
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-29 02:37 PDT by Mark Lam
Modified: 2012-08-29 17:29 PDT (History)
5 users (show)

See Also:


Attachments
Fix. (24.07 KB, patch)
2012-08-29 05:02 PDT, Mark Lam
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Fixed typo. (24.07 KB, patch)
2012-08-29 05:33 PDT, Mark Lam
ggaren: review-
Details | Formatted Diff | Diff
Removed s_isInitialized and its references. (23.54 KB, patch)
2012-08-29 08:45 PDT, Mark Lam
ggaren: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2012-08-29 02:37:08 PDT
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.
Comment 1 Mark Lam 2012-08-29 05:02:22 PDT
Created attachment 161189 [details]
Fix.
Comment 2 Gyuyoung Kim 2012-08-29 05:24:47 PDT
Comment on attachment 161189 [details]
Fix.

Attachment 161189 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13687197
Comment 3 Early Warning System Bot 2012-08-29 05:29:07 PDT
Comment on attachment 161189 [details]
Fix.

Attachment 161189 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13688184
Comment 4 Early Warning System Bot 2012-08-29 05:29:21 PDT
Comment on attachment 161189 [details]
Fix.

Attachment 161189 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13686170
Comment 5 Mark Lam 2012-08-29 05:33:49 PDT
Created attachment 161195 [details]
Fixed typo.
Comment 6 Geoffrey Garen 2012-08-29 08:08:03 PDT
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.
Comment 7 Mark Lam 2012-08-29 08:45:49 PDT
Created attachment 161237 [details]
Removed s_isInitialized and its references.
Comment 8 Geoffrey Garen 2012-08-29 10:04:37 PDT
Comment on attachment 161237 [details]
Removed s_isInitialized and its references.

r=me
Comment 9 WebKit Review Bot 2012-08-29 14:13:08 PDT
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
Comment 10 Geoffrey Garen 2012-08-29 14:19:45 PDT
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.
Comment 11 WebKit Review Bot 2012-08-29 17:24:26 PDT
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
Comment 12 Gavin Barraclough 2012-08-29 17:27:47 PDT
Landed in r127068.
Comment 13 Gavin Barraclough 2012-08-29 17:29:21 PDT
This is a JSC change, so the EWS failure on chromium must be a flake.