Bug 150792 - [ES6] Implement LLInt/Baseline Support for ES6 Generators and enable this feature
Summary: [ES6] Implement LLInt/Baseline Support for ES6 Generators and enable this fea...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 151500 151734
Blocks: 150290
  Show dependency treegraph
 
Reported: 2015-11-01 23:04 PST by Yusuke Suzuki
Modified: 2015-12-02 02:39 PST (History)
5 users (show)

See Also:


Attachments
Patch (42.52 KB, patch)
2015-11-07 11:32 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (46.52 KB, patch)
2015-11-08 04:23 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (76.30 KB, patch)
2015-11-08 07:13 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (84.25 KB, patch)
2015-11-08 07:58 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (81.59 KB, patch)
2015-11-08 10:43 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (85.81 KB, patch)
2015-11-09 10:22 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (105.05 KB, patch)
2015-11-11 10:48 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (110.29 KB, patch)
2015-11-11 11:22 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (112.25 KB, patch)
2015-11-13 11:46 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (112.42 KB, patch)
2015-11-14 04:02 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (119.31 KB, patch)
2015-11-14 11:05 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (123.11 KB, patch)
2015-11-14 23:17 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (161.48 KB, patch)
2015-11-16 04:47 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (178.90 KB, patch)
2015-11-17 10:47 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (183.63 KB, patch)
2015-11-17 11:43 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (207.73 KB, patch)
2015-11-19 13:40 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (212.42 KB, patch)
2015-11-20 07:45 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (228.22 KB, patch)
2015-11-22 05:23 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (252.49 KB, patch)
2015-11-22 10:36 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (307.48 KB, patch)
2015-11-22 11:32 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (294.23 KB, patch)
2015-11-22 11:40 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (861.80 KB, application/zip)
2015-11-22 12:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (832.89 KB, application/zip)
2015-11-22 12:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (959.35 KB, application/zip)
2015-11-22 12:48 PST, Build Bot
no flags Details
Patch (294.97 KB, patch)
2015-11-22 13:16 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (290.01 KB, patch)
2015-11-22 21:47 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (289.40 KB, patch)
2015-11-22 22:15 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (289.14 KB, patch)
2015-11-22 22:43 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (288.74 KB, patch)
2015-11-23 00:36 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (290.54 KB, patch)
2015-11-23 02:53 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (332.11 KB, patch)
2015-11-24 02:16 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (318.28 KB, patch)
2015-11-24 02:17 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (316.72 KB, patch)
2015-11-24 06:07 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (316.71 KB, patch)
2015-11-24 06:10 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (318.06 KB, patch)
2015-11-25 05:27 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (318.02 KB, patch)
2015-11-25 07:29 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (325.18 KB, patch)
2015-11-29 10:22 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (332.70 KB, patch)
2015-12-01 01:20 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (332.86 KB, patch)
2015-12-01 01:33 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (333.87 KB, patch)
2015-12-01 18:40 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-11-01 23:04:28 PST
...
Comment 1 Yusuke Suzuki 2015-11-07 11:32:03 PST
Created attachment 265004 [details]
Patch

WIP: Just add necessary stubs
Comment 2 Yusuke Suzuki 2015-11-08 04:23:26 PST
Created attachment 265015 [details]
Patch

WIP: part 2
Comment 3 Yusuke Suzuki 2015-11-08 07:13:46 PST
Created attachment 265017 [details]
Patch

WIP: part 3
Comment 4 Yusuke Suzuki 2015-11-08 07:58:26 PST
Created attachment 265018 [details]
Patch

WIP: part 4
Comment 5 Yusuke Suzuki 2015-11-08 10:43:37 PST
Created attachment 265020 [details]
Patch

WIP: part 5
Comment 6 Yusuke Suzuki 2015-11-09 10:22:27 PST
Created attachment 265069 [details]
Patch

WIP: part 6
Comment 7 Yusuke Suzuki 2015-11-11 10:48:18 PST
Created attachment 265306 [details]
Patch

WIP: part 7
Comment 8 Yusuke Suzuki 2015-11-11 11:22:27 PST
Created attachment 265310 [details]
Patch

WIP: part 8
Comment 9 Yusuke Suzuki 2015-11-11 11:33:01 PST
Comment on attachment 265310 [details]
Patch

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

For MTG discussion, I left some notes :) This patch is still highly development phase.

> Source/JavaScriptCore/runtime/GeneratorState.h:33
> +class GeneratorState : public JSCell {

This should store the current Frame.
I'm still considering which is the best; storing Frame into a generator object directly OR allocating this separately like this.
As a first step, I'm now implementing naive approach. But later, I'll figure out which design is better, may change the patch and submit it for reviews.

> Source/JavaScriptCore/runtime/GeneratorState.h:50
> +        return bitwise_cast<WriteBarrierBase<Unknown>*>(bitwise_cast<char*>(this) + offsetOfVariables());

I'm now planning to store registers as WriteBarrierBase<Unknown>.
So when doing this in FTL / DFG, we need to unbox. Value recovery information would help it.
Storing this as JSValue makes two things easy. (1) when storing, we correctly activate write barriers. (2) visitChildren easily marks all.
Comment 10 Yusuke Suzuki 2015-11-13 11:46:04 PST
Created attachment 265486 [details]
Patch

WIP: part 9. super naive prototype. there are many bugs, missing parts and inefficient parts. but small example works.
Comment 11 Saam Barati 2015-11-13 12:44:56 PST
(In reply to comment #10)
> Created attachment 265486 [details]
> Patch
> 
> WIP: part 9. super naive prototype. there are many bugs, missing parts and
> inefficient parts. but small example works.

👍
Comment 12 Yusuke Suzuki 2015-11-14 04:02:13 PST
Created attachment 265539 [details]
Patch

WIP: part 10
Comment 13 Yusuke Suzuki 2015-11-14 11:05:30 PST
Created attachment 265545 [details]
Patch

WIP: part 11, return/throw support.
Comment 14 Yusuke Suzuki 2015-11-14 23:17:09 PST
Created attachment 265558 [details]
Patch

WIP: part 12, test enable part 1
Comment 15 Yusuke Suzuki 2015-11-16 04:47:34 PST
Created attachment 265583 [details]
Patch

WIP: part 13, almost all compat-table tests are passed. still several edge cases are not covered.
Comment 16 Yusuke Suzuki 2015-11-17 10:47:13 PST
Created attachment 265681 [details]
Patch

WIP: part 14. Add new_generator_func etc. bytecodes.
Comment 17 Yusuke Suzuki 2015-11-17 11:43:06 PST
Created attachment 265686 [details]
Patch

WIP: part 15. Support throw/return before first calls. Add tests.
Comment 18 Yusuke Suzuki 2015-11-19 13:40:59 PST
Created attachment 265895 [details]
Patch

WIP: part 16. Except for correct 'super' reference, all the functionality is implemented. Code cleanup, some optimization is needed.
Comment 19 Yusuke Suzuki 2015-11-20 07:45:12 PST
Created attachment 265954 [details]
Patch

WIP: part 17. Correctly handle incorrect js values in stack.
Comment 20 Yusuke Suzuki 2015-11-22 05:23:12 PST
Created attachment 266054 [details]
Patch

WIP: part 18. Correctly handle eval('this').
Comment 21 Yusuke Suzuki 2015-11-22 10:36:20 PST
Created attachment 266056 [details]
Patch

WIP: part 19. Done, let's tweak OSX and Windows project files.
Comment 22 Yusuke Suzuki 2015-11-22 10:37:47 PST
(In reply to comment #11)
> (In reply to comment #10)
> > Created attachment 265486 [details]
> > Patch
> > 
> > WIP: part 9. super naive prototype. there are many bugs, missing parts and
> > inefficient parts. but small example works.
> 
> 👍

:D
Comment 23 Yusuke Suzuki 2015-11-22 11:32:35 PST
Created attachment 266059 [details]
Patch
Comment 24 WebKit Commit Bot 2015-11-22 11:34:07 PST
Attachment 266059 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:26:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:29:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:30:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:32:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:33:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:34:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:36:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:37:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:38:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:39:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:40:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:43:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:44:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:46:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:47:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:48:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:49:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:50:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:51:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:52:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:53:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:54:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:55:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:56:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:58:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:59:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:60:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:61:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:62:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:63:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:64:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:65:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:66:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:68:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:69:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:70:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:71:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:72:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:74:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:75:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 47 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Yusuke Suzuki 2015-11-22 11:40:08 PST
Created attachment 266060 [details]
Patch
Comment 26 WebKit Commit Bot 2015-11-22 11:43:16 PST
Attachment 266060 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:26:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:29:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:30:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:32:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:33:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:34:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:36:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:37:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:38:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:39:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:40:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:43:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:44:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:46:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:47:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:48:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:49:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:50:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:51:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:52:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:53:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:54:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:55:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:56:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:58:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:59:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:60:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:61:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:62:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:63:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:64:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:65:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:66:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:68:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:69:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:70:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:71:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:72:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:74:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:75:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 46 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Build Bot 2015-11-22 12:44:16 PST
Comment on attachment 266060 [details]
Patch

Attachment 266060 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/464496

New failing tests:
js/dom/reserved-words-as-property.html
Comment 28 Build Bot 2015-11-22 12:44:20 PST
Created attachment 266063 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 29 Build Bot 2015-11-22 12:46:05 PST
Comment on attachment 266060 [details]
Patch

Attachment 266060 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/464494

New failing tests:
js/dom/reserved-words-as-property.html
Comment 30 Build Bot 2015-11-22 12:46:09 PST
Created attachment 266064 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 31 Build Bot 2015-11-22 12:48:40 PST
Comment on attachment 266060 [details]
Patch

Attachment 266060 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/464500

New failing tests:
js/dom/reserved-words-as-property.html
Comment 32 Build Bot 2015-11-22 12:48:43 PST
Created attachment 266065 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 33 Yusuke Suzuki 2015-11-22 13:16:26 PST
Created attachment 266066 [details]
Patch
Comment 34 WebKit Commit Bot 2015-11-22 13:18:43 PST
Attachment 266066 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Yusuke Suzuki 2015-11-22 18:18:39 PST
Comment on attachment 266066 [details]
Patch

I'll move more ops to Generator.prototype side.
Comment 36 Yusuke Suzuki 2015-11-22 21:47:03 PST
Created attachment 266076 [details]
Patch
Comment 37 WebKit Commit Bot 2015-11-22 21:48:53 PST
Attachment 266076 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Yusuke Suzuki 2015-11-22 22:15:40 PST
Created attachment 266077 [details]
Patch
Comment 39 WebKit Commit Bot 2015-11-22 22:18:31 PST
Attachment 266077 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Yusuke Suzuki 2015-11-22 22:23:27 PST
Ready for review!
Comment 41 Yusuke Suzuki 2015-11-22 22:43:15 PST
Created attachment 266078 [details]
Patch

minor style tweak
Comment 42 WebKit Commit Bot 2015-11-22 22:46:00 PST
Attachment 266078 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 Yusuke Suzuki 2015-11-23 00:32:19 PST
Comment on attachment 266078 [details]
Patch

More tweak.
Comment 44 Yusuke Suzuki 2015-11-23 00:36:47 PST
Created attachment 266080 [details]
Patch
Comment 45 WebKit Commit Bot 2015-11-23 00:39:40 PST
Attachment 266080 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 46 Yusuke Suzuki 2015-11-23 02:53:22 PST
Created attachment 266083 [details]
Patch
Comment 47 WebKit Commit Bot 2015-11-23 02:55:58 PST
Attachment 266083 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 105 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 48 Saam Barati 2015-11-23 19:15:18 PST
Comment on attachment 266083 [details]
Patch

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

LGTM so far.
I still have a lot of the interesting code to review closely.
Just posting a few comments, mostly stylistic, and then I'll post more comments as I review the patch further.

> Source/JavaScriptCore/ChangeLog:45
> +                    sentValue = @generatorValue;

is sentValue what is passed into next?
i.e, next(@generatorValue)?

> Source/JavaScriptCore/builtins/Generator.prototype.js:26
> +function next(sentValue)

All three of these functions are identical except for a difference in GeneratorResumeMode.
Is it worth creating a common implementation? Is that even doable? Would it be too slow?
Maybe we want different functions for optimization reasons.
What do you think?

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:243
> +            functor(codeBlock, instruction, opcodeID, virtualRegisterForLocal(local).offset());

This seems like it might be wrong in certain situations for the same reasons you've stated
for needing the isLiveCell function. I'm not sure if it's a problem if this is exact or if it can
be a conservative estimate.

> Source/JavaScriptCore/bytecode/CodeBlock.h:661
> +    int32_t addLiveCalleeRegistersAtYield(FastBitVector vector) { createRareDataIfNecessary(); m_rareData->m_liveCalleeRegistersAtYield.append(vector); return m_rareData->m_liveCalleeRegistersAtYield.size() - 1; }

style: lets make this function have  in-between newlines.

> Source/JavaScriptCore/bytecode/CodeBlock.h:662
> +    const FastBitVector& liveCalleeRegistersAtYield(int index) { RELEASE_ASSERT(m_rareData); return m_rareData->m_liveCalleeRegistersAtYield[index]; }

I wonder if a better name for liveCalleeRegistersXXX would be "liveCalleeLocalsXXX" or "liveCalleeTempsXXX"
Whenever I think about the phrase Callee Registers I think about machine registers, not VirtualRegisters.
I'm not sure what the precedent for this is inside JSC. What do you think? I know we call it numCalleeRegisters() above, so this name is consistent with that name.
FWIW, I think that name is also kind of weird, and would sound better as numCalleeLocals or numCalleeTemps or something similar.

> Source/JavaScriptCore/bytecode/ExecutableInfo.h:34
> +// FIXME: These flags, ParserModes and propagation to XXXCodeBlocks should be reorganized.

👍

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:254
> +    int32_t addLiveCalleeRegistersAtYield(FastBitVector vector) { createRareDataIfNecessary(); m_rareData->m_liveCalleeRegistersAtYield.append(vector); return m_rareData->m_liveCalleeRegistersAtYield.size() - 1; }

style: this probably doesn't matter much but you can make this "unsigned" and instead of using Instruction.operand you can use Instruction.unsignedValue

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:521
> +        // FIXME: Emit to_this only when Generator uses it.

It's worth having a bug # here.

> Source/JavaScriptCore/parser/Parser.cpp:329
> +

nit: new whitespace

> Source/JavaScriptCore/parser/Parser.cpp:492
> +    info.parameterCount = 4; // generator, state, value, type

type => resume mode

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:98
> +        program = makeString("{function ", (isGenerator) ? "*" : "", functionName.string(), "() {\n\n}}");

style: I don't think you need parens around isGenerator.

> Source/JavaScriptCore/runtime/GeneratorFrame.h:65
> +    static ptrdiff_t offsetOfRegister(size_t index)

This is unused.

> Source/JavaScriptCore/runtime/GeneratorFunctionConstructor.cpp:56
> +    return JSValue::encode(constructFunction(exec, asInternalFunction(exec->callee())->globalObject(), args, /* isGenerator */ true));

style: I think we should use an enum class for isGenerator instead of a boolean.
That would solve having to add the comment: "/* isGenerator */"

> Source/JavaScriptCore/runtime/GeneratorThis.h:54
> +// The following flag manages this state. When GeneratorFunction is called as a constructor, it returnes a Generator that function has GeneratorThis::Empty flag.

typo: "returnes" => "returns"

> Source/JavaScriptCore/runtime/GeneratorThis.h:56
> +enum class GeneratorThis : unsigned {

Style: I think a better name for this would be GeneratorThisMode
Comment 49 Yusuke Suzuki 2015-11-24 02:02:35 PST
Comment on attachment 266083 [details]
Patch

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

Thank you for your great comments! I've reconsidered my design. I'll upload the revised patch, that leverages BytecodeLivenessAnalysis and it becomes much much cleaner :)

>> Source/JavaScriptCore/ChangeLog:45
>> +                    sentValue = @generatorValue;
> 
> is sentValue what is passed into next?
> i.e, next(@generatorValue)?

Yup. I've noted it :)

>> Source/JavaScriptCore/builtins/Generator.prototype.js:26
>> +function next(sentValue)
> 
> All three of these functions are identical except for a difference in GeneratorResumeMode.
> Is it worth creating a common implementation? Is that even doable? Would it be too slow?
> Maybe we want different functions for optimization reasons.
> What do you think?

They are slightly different (for example, in throw, when seeing Completed state, it throws.), but not so much different.
So, to check the difference, I've created the merged function. The result seems not so large and I believe it's ok for performance.
I'll update this patch with it :)

Only one concern is, of cource, the merged function is larger than this original `next`. Compared to `throw` and `return`, `next` should be so frequently called.
Making `next` small is good for DFG's inlining decision. (But seeing the result, I think it's not so large).
So I'll upload the revised one and I would like to hear your opinion.

> Source/JavaScriptCore/builtins/Generator.prototype.js:58
> +    return { done, value };

This is intentional. Only one object allocation site makes object allocation sinking easier.

>> Source/JavaScriptCore/bytecode/CodeBlock.h:661
>> +    int32_t addLiveCalleeRegistersAtYield(FastBitVector vector) { createRareDataIfNecessary(); m_rareData->m_liveCalleeRegistersAtYield.append(vector); return m_rareData->m_liveCalleeRegistersAtYield.size() - 1; }
> 
> style: lets make this function have  in-between newlines.

Inserted :)

>> Source/JavaScriptCore/bytecode/CodeBlock.h:662
>> +    const FastBitVector& liveCalleeRegistersAtYield(int index) { RELEASE_ASSERT(m_rareData); return m_rareData->m_liveCalleeRegistersAtYield[index]; }
> 
> I wonder if a better name for liveCalleeRegistersXXX would be "liveCalleeLocalsXXX" or "liveCalleeTempsXXX"
> Whenever I think about the phrase Callee Registers I think about machine registers, not VirtualRegisters.
> I'm not sure what the precedent for this is inside JSC. What do you think? I know we call it numCalleeRegisters() above, so this name is consistent with that name.
> FWIW, I think that name is also kind of weird, and would sound better as numCalleeLocals or numCalleeTemps or something similar.

Locals looks significantly nice.
"callee registers" is confusing to me too :). I've replaced this to `CalleeLocals` and `numCalleeRegisters` to `numCalleeLocals`.

>> Source/JavaScriptCore/bytecode/ExecutableInfo.h:34
>> +// FIXME: These flags, ParserModes and propagation to XXXCodeBlocks should be reorganized.
> 
> 👍

Yeah!

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:254
>> +    int32_t addLiveCalleeRegistersAtYield(FastBitVector vector) { createRareDataIfNecessary(); m_rareData->m_liveCalleeRegistersAtYield.append(vector); return m_rareData->m_liveCalleeRegistersAtYield.size() - 1; }
> 
> style: this probably doesn't matter much but you can make this "unsigned" and instead of using Instruction.operand you can use Instruction.unsignedValue

That's nice. Changed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:521
>> +        // FIXME: Emit to_this only when Generator uses it.
> 
> It's worth having a bug # here.

Nice. I've created the bug and pasted it here.
https://bugs.webkit.org/show_bug.cgi?id=151586

>> Source/JavaScriptCore/parser/Parser.cpp:329
>> +
> 
> nit: new whitespace

Removed :)

>> Source/JavaScriptCore/parser/Parser.cpp:492
>> +    info.parameterCount = 4; // generator, state, value, type
> 
> type => resume mode

Fixed :)

>> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:98
>> +        program = makeString("{function ", (isGenerator) ? "*" : "", functionName.string(), "() {\n\n}}");
> 
> style: I don't think you need parens around isGenerator.

Fixed.

>> Source/JavaScriptCore/runtime/GeneratorFrame.h:65
>> +    static ptrdiff_t offsetOfRegister(size_t index)
> 
> This is unused.

Dropped.

>> Source/JavaScriptCore/runtime/GeneratorFunctionConstructor.cpp:56
>> +    return JSValue::encode(constructFunction(exec, asInternalFunction(exec->callee())->globalObject(), args, /* isGenerator */ true));
> 
> style: I think we should use an enum class for isGenerator instead of a boolean.
> That would solve having to add the comment: "/* isGenerator */"

Added `FunctionConstructionMode` and use it :)

>> Source/JavaScriptCore/runtime/GeneratorThis.h:54
>> +// The following flag manages this state. When GeneratorFunction is called as a constructor, it returnes a Generator that function has GeneratorThis::Empty flag.
> 
> typo: "returnes" => "returns"

Nice! Fixed.

>> Source/JavaScriptCore/runtime/GeneratorThis.h:56
>> +enum class GeneratorThis : unsigned {
> 
> Style: I think a better name for this would be GeneratorThisMode

Renamed :)
Comment 50 Yusuke Suzuki 2015-11-24 02:16:01 PST
Created attachment 266127 [details]
Patch
Comment 51 Yusuke Suzuki 2015-11-24 02:17:58 PST
Created attachment 266128 [details]
Patch
Comment 52 Yusuke Suzuki 2015-11-24 02:19:29 PST
https://goo.gl/photos/CnTvLR1Z7wtd7JiDA figure to explain how save / resume works with use-def analysis.
Comment 53 Yusuke Suzuki 2015-11-24 06:07:06 PST
Created attachment 266134 [details]
Patch
Comment 54 WebKit Commit Bot 2015-11-24 06:09:51 PST
Attachment 266134 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1777:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 55 Yusuke Suzuki 2015-11-24 06:10:44 PST
Created attachment 266135 [details]
Patch
Comment 56 WebKit Commit Bot 2015-11-24 06:13:06 PST
Attachment 266135 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1777:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 57 Yusuke Suzuki 2015-11-24 06:33:22 PST
Comment on attachment 266083 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:243
>> +            functor(codeBlock, instruction, opcodeID, virtualRegisterForLocal(local).offset());
> 
> This seems like it might be wrong in certain situations for the same reasons you've stated
> for needing the isLiveCell function. I'm not sure if it's a problem if this is exact or if it can
> be a conservative estimate.

Nice catch. That's a problem.
I've changed the design slightlhy. op_save only uses a first generator and defines nothing.
And op_resume defines all the used variables at the head of the successor basic block.
I added the virtual jump edge from op_save to the point that is called the merge point.
And op_resume is also has the flow to this merge point.

            (global generator switch) -> (initial sequence) -> (op_save) ----+-> (merge point) -> (next sequence)*
                   |                                              |          |
                   |                                              v          |
                   |                                           (op_ret)      |
                   |                                                         |
                   +------------------------------------------->(op_resume)--+

By constructing such a graph,

1. Since we have a flow from (op_save) to (merge point), at merge point, we can *use* locals that are defined before (op_save)
2. op_save should claim that it does not define anything.
3. at op_resume, we see *use*d locals at merge point and define all of them.

We can do the above things in use-def analysis because use-def analysis is backward analysis.
And after analyzing use-def chains, in op_save / op_resume we only save / resume live registers at the head of merge point.
Comment 58 Yusuke Suzuki 2015-11-25 05:27:19 PST
Created attachment 266152 [details]
Patch
Comment 59 WebKit Commit Bot 2015-11-25 05:29:05 PST
Attachment 266152 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1783:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 60 Yusuke Suzuki 2015-11-25 07:29:32 PST
Created attachment 266161 [details]
Patch
Comment 61 WebKit Commit Bot 2015-11-25 07:32:37 PST
Attachment 266161 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1783:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 62 Yusuke Suzuki 2015-11-26 01:54:31 PST
Comment on attachment 266161 [details]
Patch

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

Added comment for ease of review.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:62
> +    case op_save:

op_save has a virtual jump to a merge point. (the virtual jump means, that is a jump edge, but it is not used actually. It is used to construct CFG, and this flow will be leveraged for DFG analysis.)

Since op_save has a virtual jump to a merge point, there is a edge from op_save to the merge point.
And the merge point can use registers that are defined predecessor basic blocks that is dominated by this op_save!

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:253
> +        return;

op_save *use*s

1. generator register to save registers to its GeneratorFrame member.
2. registers that are used at the merge point. Since use-def is backward analysis, when analyzing op_save, its information is already analyzed.

And op_save *def*ines

1. Nothing

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:421
> +        return;

op_resume *use*s

1. generator register to resume registers from GeneratorFrame

And op_resume *def*ines

2. registers that are used at the merge point.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1550
> +        }

And added liveness information to bytecode dump. When you see bytecode with `-d`, you can see which registers will be saved / resumed at that op_save / op_resume.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2275
> +    }

This is important part. We perform BytecodeLivenessAnalysis to find which register is live at a given merge point.
This information will be used to save / resume registers.

And interestinglly, in typical cases, the number of live registers is very small.
We will emit asm directly for this save / resume operation in DFG/FTL! This enables significantly fast generator operations.
Comment 63 Yusuke Suzuki 2015-11-26 01:55:38 PST
Comment on attachment 266161 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:62
>> +    case op_save:
> 
> op_save has a virtual jump to a merge point. (the virtual jump means, that is a jump edge, but it is not used actually. It is used to construct CFG, and this flow will be leveraged for DFG analysis.)
> 
> Since op_save has a virtual jump to a merge point, there is a edge from op_save to the merge point.
> And the merge point can use registers that are defined predecessor basic blocks that is dominated by this op_save!

s/dominated/that will reach to the merge point through this op_save/.
Comment 64 Saam Barati 2015-11-28 18:54:39 PST
Comment on attachment 266161 [details]
Patch

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

LGTM, just a few comments. I have some more comments I'll write tomorrow but I'm about to board a flight.

> Source/JavaScriptCore/ChangeLog:79
> +        Registers reside in C stack. So if we don't perform it, we need to determine whether a given value is *live* and *correct* JSCell or not

I'm not sure if this last sentence is necessary and it's a bit confusing to me. I think the above sentence saying that we run BytecodeLiveness is enough to get this idea across
that we need to know which registers to save and restore.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2247
> +            mergePointBytecodeOffsets.resize(liveCalleeLocalsIndex + 1);

Nit: Even though this number will always get larger because of how we parse/generate bytecode, we really should only resize when we're resizing to a larger number than the current size.
It might be worth making that explicit either through an assert/if statement/max function call.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4128
> +    RegisterID* completedState = emitLoad(nullptr, jsNumber(static_cast<int32_t>(state)));

style: cast not needed

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3006
> +        generator.emitProfileType(generator.generatorRegister(), ProfileTypeBytecodeFunctionReturnStatement); // Do not emit expression info for this profile because it's not in the user's source code.

I don't think this is needed.

> Source/JavaScriptCore/tests/es6.yaml:841
>  - path: es6/generators_yield_star_instances_of_iterables.js

Can we also add some of our own "yeild*" tests with different variations?

> Source/JavaScriptCore/tests/stress/generator-return-before-first-call.js:1
> +function unreachable()

I think it's worth having more return(X) and throw(X) tests.

> Source/JavaScriptCore/tests/stress/generator-return-before-first-call.js:12
> +g.return("Hello");

We should have tests for the return value here.
Comment 65 Yusuke Suzuki 2015-11-29 10:22:09 PST
Created attachment 266224 [details]
Patch

Update yield*, added tests, but still return/throw tests are not added yet.
Comment 66 WebKit Commit Bot 2015-11-29 10:25:12 PST
Attachment 266224 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1783:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 118 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 67 Yusuke Suzuki 2015-12-01 01:20:33 PST
Created attachment 266343 [details]
Patch
Comment 68 WebKit Commit Bot 2015-12-01 01:24:05 PST
Attachment 266343 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1792:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 120 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 69 Yusuke Suzuki 2015-12-01 01:33:36 PST
Created attachment 266346 [details]
Patch
Comment 70 WebKit Commit Bot 2015-12-01 01:35:25 PST
Attachment 266346 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1792:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 120 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 71 Saam Barati 2015-12-01 09:20:28 PST
Comment on attachment 266346 [details]
Patch

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

r=me with a couple comments

> Source/JavaScriptCore/bytecode/PreciseJumpTargets.cpp:57
> +    case op_save:

Maybe a small comment is worth it here just to say that this is purely
for calculating liveness.

> Source/JavaScriptCore/parser/ParserModes.h:50
> +    GeneratorFunctionMode,

GeneratorMode and GeneratorFunctionMode confuse me a bit as names. 
I wonder if we can name them better to be more indicative of what they do
Maybe GeneratorBodyMode instead of GeneratorMode. Maybe GeneratorWrapperFunctionMode
instead of GeneratorFunctionMode?
Comment 72 Yusuke Suzuki 2015-12-01 13:53:11 PST
Comment on attachment 266161 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:79
>> +        Registers reside in C stack. So if we don't perform it, we need to determine whether a given value is *live* and *correct* JSCell or not
> 
> I'm not sure if this last sentence is necessary and it's a bit confusing to me. I think the above sentence saying that we run BytecodeLiveness is enough to get this idea across
> that we need to know which registers to save and restore.

Yes. Running BytecodeLiveness is enough. Dropped this sequence.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2247
>> +            mergePointBytecodeOffsets.resize(liveCalleeLocalsIndex + 1);
> 
> Nit: Even though this number will always get larger because of how we parse/generate bytecode, we really should only resize when we're resizing to a larger number than the current size.
> It might be worth making that explicit either through an assert/if statement/max function call.

OK, I've added one if statement that avoid `resize` when vector size is enough for a given index.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4128
>> +    RegisterID* completedState = emitLoad(nullptr, jsNumber(static_cast<int32_t>(state)));
> 
> style: cast not needed

Nice! Dropped.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3006
>> +        generator.emitProfileType(generator.generatorRegister(), ProfileTypeBytecodeFunctionReturnStatement); // Do not emit expression info for this profile because it's not in the user's source code.
> 
> I don't think this is needed.

OK, dropped.

>> Source/JavaScriptCore/tests/es6.yaml:841
>>  - path: es6/generators_yield_star_instances_of_iterables.js
> 
> Can we also add some of our own "yeild*" tests with different variations?

Added :)

>> Source/JavaScriptCore/tests/stress/generator-return-before-first-call.js:1
>> +function unreachable()
> 
> I think it's worth having more return(X) and throw(X) tests.

Added :)

>> Source/JavaScriptCore/tests/stress/generator-return-before-first-call.js:12
>> +g.return("Hello");
> 
> We should have tests for the return value here.

Added :D
Comment 73 Yusuke Suzuki 2015-12-01 13:59:56 PST
Comment on attachment 266346 [details]
Patch

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

Thanks for your great review!

>> Source/JavaScriptCore/bytecode/PreciseJumpTargets.cpp:57
>> +    case op_save:
> 
> Maybe a small comment is worth it here just to say that this is purely
> for calculating liveness.

Sounds nice. Added a small comment.

>> Source/JavaScriptCore/parser/ParserModes.h:50
>> +    GeneratorFunctionMode,
> 
> GeneratorMode and GeneratorFunctionMode confuse me a bit as names. 
> I wonder if we can name them better to be more indicative of what they do
> Maybe GeneratorBodyMode instead of GeneratorMode. Maybe GeneratorWrapperFunctionMode
> instead of GeneratorFunctionMode?

Your suggestion looks nice. More descriptive name is nice for readability :D Changed.
Comment 74 Yusuke Suzuki 2015-12-01 14:23:36 PST
Committed r192914: <http://trac.webkit.org/changeset/192914>
Comment 75 WebKit Commit Bot 2015-12-01 17:33:22 PST
Re-opened since this is blocked by bug 151734
Comment 77 Yusuke Suzuki 2015-12-01 18:40:31 PST
Created attachment 266421 [details]
Patch
Comment 78 WebKit Commit Bot 2015-12-01 18:43:09 PST
Attachment 266421 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1792:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 120 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 79 Yusuke Suzuki 2015-12-01 18:49:06 PST
Comment on attachment 266421 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:-229
> -#endif

Drop this. Now create_this can be used for non constructor.
Comment 80 Saam Barati 2015-12-01 18:55:13 PST
Comment on attachment 266421 [details]
Patch

r=me
Comment 81 Yusuke Suzuki 2015-12-01 19:16:49 PST
Committed r192937: <http://trac.webkit.org/changeset/192937>
Comment 82 Yusuke Suzuki 2015-12-02 02:27:18 PST
oops, Windows build's ENABLE_ES6_GENERATORS is not enabled.
I'll land the follow up patch that enables ENABLE_ES6_GENERATORS. (Maybe, CMake)
Comment 83 Yusuke Suzuki 2015-12-02 02:38:21 PST
Hmm, OptionsWin.cmake is correctly updated...
I've just requested Win force rebuild.
Comment 84 Yusuke Suzuki 2015-12-02 02:39:25 PST
OK, it seems that ENABLE_ES6_MODULES is enabled.