Summary: | DFG should emit inline code for property storage (re)allocation | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | barraclough, fpizlo, ggaren, mhahnenberg, oliver, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Filip Pizlo
2012-07-18 00:34:33 PDT
Created attachment 152957 [details]
work in progress
I don't know if it even compiles. And I haven't written the 32-bit code yet.
Comment on attachment 152957 [details] work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=152957&action=review > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:592 > + // If we can't prove that this changes the storage of our object, > + // then we must conservatively assume that it might change it. Of I think this comment is missing a "not" somewhere. (In reply to comment #2) > (From update of attachment 152957 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152957&action=review > > > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:592 > > + // If we can't prove that this changes the storage of our object, > > + // then we must conservatively assume that it might change it. Of > > I think this comment is missing a "not" somewhere. I think it's correct. If child1 isn't matched then we must prevent CSE of GetPropertyStorage from working in case our child1 and their child1 was the same object but we didn't know it at compile time. Comment on attachment 152957 [details] work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=152957&action=review >>> Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:592 >>> + // then we must conservatively assume that it might change it. Of >> >> I think this comment is missing a "not" somewhere. > > I think it's correct. If child1 isn't matched then we must prevent CSE of GetPropertyStorage from working in case our child1 and their child1 was the same object but we didn't know it at compile time. How about this: // If we can cheaply prove this is a change to our object's storage, we can optimize and use its result. if (node.child1() == child1) return index; // Otherwise, this still *might* change our object's storage, and we don't know what it changes to. return NoNode; (In reply to comment #4) > (From update of attachment 152957 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152957&action=review > > >>> Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:592 > >>> + // then we must conservatively assume that it might change it. Of > >> > >> I think this comment is missing a "not" somewhere. > > > > I think it's correct. If child1 isn't matched then we must prevent CSE of GetPropertyStorage from working in case our child1 and their child1 was the same object but we didn't know it at compile time. > > How about this: > > // If we can cheaply prove this is a change to our object's storage, we can optimize and use its result. > if (node.child1() == child1) > return index; > > // Otherwise, this still *might* change our object's storage, and we don't know what it changes to. > return NoNode; That's slightly better though the second comment is likely clearer (and more inline with aliasing terminology) as: "Otherwise, we currently can't prove that this doesn't change our object's storage, so we conservatively assume that it may change the storage pointer of any object, including ours." Created attachment 153099 [details]
the patch
Attachment 153099 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:107: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:108: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 2 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 153101 [details]
the patch
Rebased.
Attachment 153101 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOperations.h:107: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.h:108: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 2 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Landed in http://trac.webkit.org/changeset/123052 |