Bug 99557 - Array and object allocations via 'new Object' or 'new Array' should be inlined in bytecode to allow allocation site profiling
Summary: Array and object allocations via 'new Object' or 'new Array' should be inline...
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: 98606
  Show dependency treegraph
 
Reported: 2012-10-17 00:35 PDT by Filip Pizlo
Modified: 2012-10-17 14:48 PDT (History)
6 users (show)

See Also:


Attachments
possibly the patch (34.95 KB, patch)
2012-10-17 00:42 PDT, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
the patch (35.04 KB, patch)
2012-10-17 02:00 PDT, Filip Pizlo
fpizlo: review-
Details | Formatted Diff | Diff
the patch (37.53 KB, patch)
2012-10-17 02:33 PDT, Filip Pizlo
ggaren: 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-10-17 00:35:57 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2012-10-17 00:42:16 PDT
Created attachment 169110 [details]
possibly the patch
Comment 2 Early Warning System Bot 2012-10-17 00:58:10 PDT
Comment on attachment 169110 [details]
possibly the patch

Attachment 169110 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14397471
Comment 3 Build Bot 2012-10-17 01:03:48 PDT
Comment on attachment 169110 [details]
possibly the patch

Attachment 169110 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14386476
Comment 4 Gyuyoung Kim 2012-10-17 01:10:05 PDT
Comment on attachment 169110 [details]
possibly the patch

Attachment 169110 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14393501
Comment 5 Early Warning System Bot 2012-10-17 01:34:30 PDT
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
Comment 6 Filip Pizlo 2012-10-17 02:00:08 PDT
Created attachment 169125 [details]
the patch

Performance neutral, appears to pass things.  Now it even works on 32-bit, too.
Comment 7 Filip Pizlo 2012-10-17 02:00:42 PDT
Comment on attachment 169125 [details]
the patch

Nah, spoke too soon.  Will investigate.
Comment 8 Filip Pizlo 2012-10-17 02:33:30 PDT
Created attachment 169136 [details]
the patch

I think it's good now.
Comment 9 Geoffrey Garen 2012-10-17 10:17:08 PDT
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.
Comment 10 Filip Pizlo 2012-10-17 14:39:57 PDT
Landed in http://trac.webkit.org/changeset/131644
Comment 11 Filip Pizlo 2012-10-17 14:42:50 PDT
(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.
Comment 12 Filip Pizlo 2012-10-17 14:48:38 PDT
(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