Bug 130756

Summary: [ftlopt] DFG bytecode parser should turn PutById with nothing but a Setter stub as stuff+handleCall, and handleCall should be allowed to inline if it wants to
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 133034, 133042, 133105    
Bug Blocks: 129588, 133052    
Attachments:
Description Flags
it begins
none
work in progress
none
done?
none
more...
none
omg it compiles
none
"getting" there
none
it's back to compiling
none
it begins (setter inlining)
none
more!
none
the code has been written
none
it compiles
none
the patch oliver: review+

Description Filip Pizlo 2014-03-25 16:31:00 PDT
...
Comment 1 Filip Pizlo 2014-05-17 22:57:42 PDT
Created attachment 231649 [details]
it begins

Saving my work while I work on one of the required bits in a separate bug: https://bugs.webkit.org/show_bug.cgi?id=133042
Comment 2 Filip Pizlo 2014-05-18 12:12:20 PDT
Created attachment 231661 [details]
work in progress
Comment 3 Filip Pizlo 2014-05-18 12:43:02 PDT
Created attachment 231662 [details]
done?
Comment 4 Filip Pizlo 2014-05-18 18:57:47 PDT
Created attachment 231671 [details]
more...
Comment 5 Filip Pizlo 2014-05-18 19:23:19 PDT
Created attachment 231672 [details]
omg it compiles
Comment 6 Filip Pizlo 2014-05-19 12:58:20 PDT
Created attachment 231713 [details]
"getting" there
Comment 7 Filip Pizlo 2014-05-19 19:13:44 PDT
Created attachment 231748 [details]
it's back to compiling
Comment 8 Filip Pizlo 2014-05-19 20:53:58 PDT
I'll go ahead and do just the getter part separately, in https://bugs.webkit.org/show_bug.cgi?id=133105
Comment 9 Filip Pizlo 2014-05-20 18:11:43 PDT
What is left is to extend this functionality to setters.
Comment 10 Filip Pizlo 2014-06-28 17:44:36 PDT
Created attachment 234056 [details]
it begins (setter inlining)
Comment 11 Filip Pizlo 2014-06-29 20:46:43 PDT
Created attachment 234065 [details]
more!
Comment 12 Filip Pizlo 2014-06-30 15:31:28 PDT
Created attachment 234105 [details]
the code has been written
Comment 13 Filip Pizlo 2014-06-30 19:16:03 PDT
Created attachment 234139 [details]
it compiles
Comment 14 Filip Pizlo 2014-07-01 13:14:26 PDT
Created attachment 234193 [details]
the patch
Comment 15 Oliver Hunt 2014-07-01 14:41:38 PDT
Comment on attachment 234193 [details]
the patch

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

> Source/JavaScriptCore/tests/stress/weird-setter-counter.js:20
> +    var o = {};
> +    o.__defineSetter__("f", function(value) {

While it logically shouldn't matter could you also add tests using {set f(value) { ... }} just to make sure we're not doing anything substantially different?
Comment 16 Filip Pizlo 2014-07-01 14:44:38 PDT
(In reply to comment #15)
> (From update of attachment 234193 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234193&action=review
> 
> > Source/JavaScriptCore/tests/stress/weird-setter-counter.js:20
> > +    var o = {};
> > +    o.__defineSetter__("f", function(value) {
> 
> While it logically shouldn't matter could you also add tests using {set f(value) { ... }} just to make sure we're not doing anything substantially different?

That makes sense.  How about I just replicate weird-setter-counter in this way, since if I replicate all of the tests in this way then we'll have too much overlapping coverage.
Comment 17 Filip Pizlo 2014-07-01 15:40:51 PDT
Landed in http://trac.webkit.org/changeset/170672