Bug 91597 – DFG should emit inline code for property storage (re)allocation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Work in progress patch forthcoming.

[Filip Pizlo]

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.

[Geoffrey Garen]

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.

[Filip Pizlo]

(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.

[Geoffrey Garen]

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;

[Filip Pizlo]

(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."

[Filip Pizlo]

Created attachment 153099 [details]
the patch

[WebKit Review Bot]

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.

[Filip Pizlo]

Created attachment 153101 [details]
the patch

Rebased.

[WebKit Review Bot]

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.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/123052