Bug 91597 - DFG should emit inline code for property storage (re)allocation
Summary: DFG should emit inline code for property storage (re)allocation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-18 00:34 PDT by Filip Pizlo
Modified: 2012-07-18 17:31 PDT (History)
6 users (show)

See Also:


Attachments
work in progress (14.80 KB, patch)
2012-07-18 00:58 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (27.74 KB, patch)
2012-07-18 15:17 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (26.76 KB, patch)
2012-07-18 15:22 PDT, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-07-18 00:34:33 PDT
Work in progress patch forthcoming.
Comment 1 Filip Pizlo 2012-07-18 00:58:01 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 2 Geoffrey Garen 2012-07-18 10:07:33 PDT
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.
Comment 3 Filip Pizlo 2012-07-18 10:14:57 PDT
(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 4 Geoffrey Garen 2012-07-18 10:58:01 PDT
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;
Comment 5 Filip Pizlo 2012-07-18 12:53:49 PDT
(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."
Comment 6 Filip Pizlo 2012-07-18 15:17:34 PDT
Created attachment 153099 [details]
the patch
Comment 7 WebKit Review Bot 2012-07-18 15:21:56 PDT
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.
Comment 8 Filip Pizlo 2012-07-18 15:22:01 PDT
Created attachment 153101 [details]
the patch

Rebased.
Comment 9 WebKit Review Bot 2012-07-18 15:24:36 PDT
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.
Comment 10 Filip Pizlo 2012-07-18 17:31:00 PDT
Landed in http://trac.webkit.org/changeset/123052