Bug 175653 - Speculatively change iteration protocall to use the same next function
Summary: Speculatively change iteration protocall to use the same next function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-16 19:42 PDT by Keith Miller
Modified: 2017-09-27 13:00 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.23 KB, patch)
2017-08-16 20:21 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (22.71 KB, patch)
2017-08-17 11:01 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.30 MB, application/zip)
2017-08-17 11:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.33 MB, application/zip)
2017-08-17 12:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.00 MB, application/zip)
2017-08-17 12:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.44 MB, application/zip)
2017-08-17 14:02 PDT, Build Bot
no flags Details
Patch (25.23 KB, patch)
2017-08-18 13:17 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (27.61 KB, patch)
2017-08-21 16:06 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (40.20 KB, patch)
2017-09-22 18:29 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2017-08-16 19:42:56 PDT
nSpeculatively change iteration protocall to use the same next function
Comment 1 Keith Miller 2017-08-16 20:21:05 PDT
Created attachment 318323 [details]
Patch
Comment 2 Saam Barati 2017-08-16 20:33:13 PDT
Comment on attachment 318323 [details]
Patch

You need to also change builtins/IteratorHelpers.js.
Maybe other places as well?

Doesn't this break some of our tests?
Comment 3 Keith Miller 2017-08-17 11:01:55 PDT
Created attachment 318386 [details]
Patch
Comment 4 Build Bot 2017-08-17 11:04:52 PDT
Attachment 318386 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/IteratorOperations.h:40:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2017-08-17 11:59:40 PDT
Comment on attachment 318386 [details]
Patch

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

New failing tests:
js/sequence-iterator-protocol-2.html
Comment 6 Build Bot 2017-08-17 11:59:43 PDT
Created attachment 318394 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-08-17 12:02:50 PDT
Comment on attachment 318386 [details]
Patch

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

New failing tests:
js/sequence-iterator-protocol-2.html
Comment 8 Build Bot 2017-08-17 12:02:52 PDT
Created attachment 318395 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-08-17 12:12:40 PDT
Comment on attachment 318386 [details]
Patch

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

New failing tests:
js/sequence-iterator-protocol-2.html
Comment 10 Build Bot 2017-08-17 12:12:42 PDT
Created attachment 318397 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-08-17 14:02:42 PDT
Comment on attachment 318386 [details]
Patch

Attachment 318386 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4332141

New failing tests:
js/sequence-iterator-protocol-2.html
Comment 12 Build Bot 2017-08-17 14:02:45 PDT
Created attachment 318415 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 13 Sam Weinig 2017-08-18 12:08:28 PDT
What is the goal of making this change?
Comment 14 Keith Miller 2017-08-18 13:05:07 PDT
(In reply to Sam Weinig from comment #13)
> What is the goal of making this change?

It's related to https://bugs.webkit.org/show_bug.cgi?id=175454, as it will simplify the implementation of that. Also, it's a possible spec change in the next version of ES6. I'm landing the patch now to see if it breaks any pages in STP, I highly doubt it will though.
Comment 15 Keith Miller 2017-08-18 13:17:55 PDT
Created attachment 318533 [details]
Patch
Comment 16 Build Bot 2017-08-18 13:19:24 PDT
Attachment 318533 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/IteratorOperations.h:40:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Saam Barati 2017-08-18 13:53:48 PDT
Comment on attachment 318533 [details]
Patch

You still need to change the builtin file that does iteration for spread
Comment 18 Keith Miller 2017-08-21 16:06:46 PDT
Created attachment 318685 [details]
Patch
Comment 19 Saam Barati 2017-08-22 11:31:12 PDT
Comment on attachment 318685 [details]
Patch

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

r=me

Please add tests that verify "next" is only accessed once per for-of, spread, and anything else that does the protocol (not sure if there is anything else off the top of my head)

> Source/JavaScriptCore/ChangeLog:13
> +        See: https://github.com/tc39/ecma262/issues/976

Let's also have a bugzilla that tracks if you haven't already made one.
Comment 20 GSkachkov 2017-08-23 10:11:24 PDT
Comment on attachment 318685 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:19
> +        (JSC::BytecodeGenerator::emitIteratorNext):

There is might be conflict after landing async iteration : https://trac.webkit.org/r221080, please do rebase.
Comment 21 Keith Miller 2017-09-22 18:29:23 PDT
Created attachment 321608 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2017-09-22 19:08:08 PDT
Comment on attachment 321608 [details]
Patch for landing

Clearing flags on attachment: 321608

Committed r222421: <http://trac.webkit.org/changeset/222421>
Comment 23 WebKit Commit Bot 2017-09-22 19:08:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Joseph Pecoraro 2017-09-23 02:56:40 PDT
It looks like this is causing test262 timeouts:
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20Test262%20%28Tests%29/builds/4314

Locally I'm seeing this test hang forever:
./test262/test/language/statements/for-of/iterator-next-reference.js
Comment 25 Keith Miller 2017-09-23 17:01:32 PDT
(In reply to Joseph Pecoraro from comment #24)
> It looks like this is causing test262 timeouts:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20Test262%20%28Tests%29/builds/4314
> 
> Locally I'm seeing this test hang forever:
> ./test262/test/language/statements/for-of/iterator-next-reference.js

Ah, I must have missed that test for some reason... I put a fix here: https://bugs.webkit.org/show_bug.cgi?id=177412
Comment 26 Radar WebKit Bug Importer 2017-09-27 13:00:12 PDT
<rdar://problem/34694460>