Bug 95316

Summary: Refactoring LLInt::Data to be a singleton
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, dglazkov, fpizlo, ggaren, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fix.
gyuyoung.kim: commit-queue-
Fixed typo.
ggaren: review-
Removed s_isInitialized and its references. ggaren: review+, webkit.review.bot: commit-queue-

Mark Lam
Reported 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.
Attachments
Fix. (24.07 KB, patch)
2012-08-29 05:02 PDT, Mark Lam 		gyuyoung.kim: commit-queue-
DetailsFormatted DiffDiff
Fixed typo. (24.07 KB, patch)
2012-08-29 05:33 PDT, Mark Lam 		ggaren: review-
DetailsFormatted DiffDiff
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-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2012-08-29 05:02:22 PDT
Created attachment 161189 [details] Fix.
Gyuyoung Kim
Comment 2 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
Early Warning System Bot
Comment 3 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
Early Warning System Bot
Comment 4 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
Mark Lam
Comment 5 2012-08-29 05:33:49 PDT
Created attachment 161195 [details] Fixed typo.
Geoffrey Garen
Comment 6 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.
Mark Lam
Comment 7 2012-08-29 08:45:49 PDT
Created attachment 161237 [details] Removed s_isInitialized and its references.
Geoffrey Garen
Comment 8 2012-08-29 10:04:37 PDT
Comment on attachment 161237 [details] Removed s_isInitialized and its references. r=me
WebKit Review Bot
Comment 9 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
Geoffrey Garen
Comment 10 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.
WebKit Review Bot
Comment 11 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
Gavin Barraclough
Comment 12 2012-08-29 17:27:47 PDT
Landed in r127068.
Gavin Barraclough
Comment 13 2012-08-29 17:29:21 PDT
This is a JSC change, so the EWS failure on chromium must be a flake.
Note You need to log in before you can comment on or make changes to this bug.