Bug 99557 – Array and object allocations via 'new Object' or 'new Array' should be inlined in bytecode to allow allocation site profiling

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 169110 [details]
possibly the patch

[Early Warning System Bot]

Comment on attachment 169110 [details]
possibly the patch

Attachment 169110 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14397471

[Build Bot]

Comment on attachment 169110 [details]
possibly the patch

Attachment 169110 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14386476

[Gyuyoung Kim]

Comment on attachment 169110 [details]
possibly the patch

Attachment 169110 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14393501

[Early Warning System Bot]

Comment on attachment 169110 [details]
possibly the patch

Attachment 169110 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14398469

[Filip Pizlo]

Created attachment 169125 [details]
the patch

Performance neutral, appears to pass things.  Now it even works on 32-bit, too.

[Filip Pizlo]

Comment on attachment 169125 [details]
the patch

Nah, spoke too soon.  Will investigate.

[Filip Pizlo]

Created attachment 169136 [details]
the patch

I think it's good now.

[Geoffrey Garen]

Comment on attachment 169136 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169136&action=review

r=me

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:438
> +        // This passes NoExpectedFunction because we expect that if the function is in a
> +        // local variable, then it's not one of our built-in constructors.

Slightly more accurate to say "then we aren't able to prove that it's one of our built-in constructors".

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:456
> +    // FIXME: Reconsider passing expectedFunction instead of NoExpectedFunction.

Are you saying we should consider *not* passing expectedFunction here? I'd suggest saying something here about why we might want to make that change. Otherwise, this comment doesn't give specific direction to another reader.

[Filip Pizlo]

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

[Filip Pizlo]

(In reply to comment #9)
> (From update of attachment 169136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169136&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:438
> > +        // This passes NoExpectedFunction because we expect that if the function is in a
> > +        // local variable, then it's not one of our built-in constructors.
> 
> Slightly more accurate to say "then we aren't able to prove that it's one of our built-in constructors".

I actually think the comment is more accurate.  Just because we resolved to a non-local variable called "Object" doesn't mean we proved that we have the Object constructor.  Likewise, just because resolved to a local variable called "Object" doesn't mean we disproved that we have the Object constructor because people could do crazy:

var x = Object;
function foo() {
   var Object = x;
   return new Object();
}

It's just a matter of likelihood.  The above snippet is less likely than someone just using the word "Object" to signify a local variable, or at least, that's what I'm assuming in this code path.

> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:456
> > +    // FIXME: Reconsider passing expectedFunction instead of NoExpectedFunction.
> 
> Are you saying we should consider *not* passing expectedFunction here? I'd suggest saying something here about why we might want to make that change. Otherwise, this comment doesn't give specific direction to another reader.

Actually this comment is just plain wrong.  I've removed the comment.

[Filip Pizlo]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 169136 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=169136&action=review
> > 
> > r=me
> > 
> > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:438
> > > +        // This passes NoExpectedFunction because we expect that if the function is in a
> > > +        // local variable, then it's not one of our built-in constructors.
> > 
> > Slightly more accurate to say "then we aren't able to prove that it's one of our built-in constructors".
> 
> I actually think the comment is more accurate.  Just because we resolved to a non-local variable called "Object" doesn't mean we proved that we have the Object constructor.  Likewise, just because resolved to a local variable called "Object" doesn't mean we disproved that we have the Object constructor because people could do crazy:
> 
> var x = Object;
> function foo() {
>    var Object = x;
>    return new Object();
> }
> 
> It's just a matter of likelihood.  The above snippet is less likely than someone just using the word "Object" to signify a local variable, or at least, that's what I'm assuming in this code path.
> 
> > 
> > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:456
> > > +    // FIXME: Reconsider passing expectedFunction instead of NoExpectedFunction.
> > 
> > Are you saying we should consider *not* passing expectedFunction here? I'd suggest saying something here about why we might want to make that change. Otherwise, this comment doesn't give specific direction to another reader.
> 
> Actually this comment is just plain wrong.  I've removed the comment.

Removed the comment in http://trac.webkit.org/changeset/131646