Bug 130756 - [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
Summary: [ftlopt] DFG bytecode parser should turn PutById with nothing but a Setter st...
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: 133034 133042 133105
Blocks: 129588 133052
  Show dependency tree / graph
 
Reported: 2014-03-25 16:31 PDT by Filip Pizlo
Modified: 2014-07-01 15:40 PDT (History)
7 users (show)

See Also:

Attachments
it begins (14.55 KB, patch)
2014-05-17 22:57 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
work in progress (31.36 KB, patch)
2014-05-18 12:12 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
done? (37.81 KB, patch)
2014-05-18 12:43 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
more... (51.42 KB, patch)
2014-05-18 18:57 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
omg it compiles (53.88 KB, patch)
2014-05-18 19:23 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
"getting" there (72.83 KB, patch)
2014-05-19 12:58 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
it's back to compiling (75.59 KB, patch)
2014-05-19 19:13 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
it begins (setter inlining) (10.30 KB, patch)
2014-06-28 17:44 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
more! (33.43 KB, patch)
2014-06-29 20:46 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the code has been written (47.84 KB, patch)
2014-06-30 15:31 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
it compiles (57.07 KB, patch)
2014-06-30 19:16 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (70.01 KB, patch)
2014-07-01 13:14 PDT, Filip Pizlo 		oliver: review+
Details | Formatted Diff | Diff
Show Obsolete (11) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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