Move away from Promise.all way and check if we need to finish manually #26395

CYBAI · 2020-05-03T02:48:39Z

In the previous Promise.all way, we registered a promise for every
module script which means we will need to do many complex checkings like
"is this top level?" and it will make us much more difficult to understand
how the module script algorithm works.

In the new manual checking way, we will only register promises for top
level modules to notify its owner (e.g. the script element) to finish
the load. So, we can understand it much more easily and would be more
spec-aligned.

Also, I think the Ready and FetchFailed status are quite confusing
and we actually don't need them so they're removed in this patch. Then,
we will always go to Finished instead.

It would basically be following steps:

                                          +-----------------+
                                          | Failed to fetch | ----------+
+--------------+       +----------+     / +-----------------+           |
| Fetch module | ----> | Fetching | ---+                                v
+--------------+       +----------+     \ +---------+        +---------------------+
                                          | Fetched |        | Advance to Finished |
                                          +---------+        +---------------------+
                                               |                        ^
                                               v                        |
                                      +-------------------+             |
                                      | Fetch descendants | ----- if no descendants
                                      +-------------------+
                                               |
                                               V
                                     +----------------------+
                                     | Fetching Descendants |
                                     +----------------------+

In Advance to Finished, it means that module script is about to finished so it will

Notify all of its ready and not finished parents to finish
Link (instantiate) the module
Resolve its promise to notify owner(s) to finish

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #25442
These changes do not require tests because it basically refactors the script module and should not break anything.

highfive · 2020-05-03T02:48:44Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/script_module.rs
@KiChjang: components/script/script_module.rs

highfive · 2020-05-03T02:48:47Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

CYBAI · 2020-05-03T02:49:15Z

also cc @Manishearth

CYBAI · 2020-05-03T03:01:47Z

@bors-servo try=wpt

bors-servo · 2020-05-03T03:01:51Z

⌛ Trying commit ca52155 with merge d64265e...


        Auto merge of #26395 - CYBAI:refactor-module-script, r=<try>

Move away from Promise.all way and check if we need to finish manually In the previous Promise.all way, we registered a promise for every module script which means we will need to do many complex checkings like "is this top level?" and it will make us much more difficult to understand how the module script algorithm works. In the new manual checking way, we will only register promises for top level modules to notify its owner (e.g. the script element) to finish the load. So, we can understand it much more easily and would be more spec-aligned. Also, I think the `Ready` and `FetchFailed` status are quite confusing and we actually don't need them so they're removed in this patch. Then, we will always go to `Finished` instead. It would basically be following steps: ``` +-----------------+ | Failed to fetch | ----------+ +--------------+ +----------+ / +-----------------+ | | Fetch module | ----> | Fetching | ---+ v +--------------+ +----------+ \ +---------+ +---------------------+ | Fetched | | Advance to Finished | +---------+ +---------------------+ | ^ v | +-------------------+ | | Fetch descendants | ----- if no descendants +-------------------+ | V +----------------------+ | Fetching Descendants | +----------------------+ ``` In `Advance to Finished`, it means that module script is about to finished so it will 1. Notify all of its `ready` and `not finished` parents to finish 2. Link (instantiate) the module 3. Resolve its promise to notify owner(s) to finish  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25442 - [x] These changes do not require tests because it basically refactors the script module and should not break anything.

bors-servo · 2020-05-03T03:44:39Z

💔 Test failed - status-taskcluster

CYBAI · 2020-05-03T07:49:32Z

@bors-servo try=wpt

I also refactored errors for module script in 2e8381d. Does that make sense to have that commit in this PR? or I can send another one which will depend on this one.

Also, I can't reproduce the failure in previous try locally... I suspect there will be some intermittent failures 😕 so, just try again.

bors-servo · 2020-05-03T07:49:37Z

⌛ Trying commit 2e8381d with merge 9424a0a...


        Auto merge of #26395 - CYBAI:refactor-module-script, r=<try>

Move away from Promise.all way and check if we need to finish manually In the previous Promise.all way, we registered a promise for every module script which means we will need to do many complex checkings like "is this top level?" and it will make us much more difficult to understand how the module script algorithm works. In the new manual checking way, we will only register promises for top level modules to notify its owner (e.g. the script element) to finish the load. So, we can understand it much more easily and would be more spec-aligned. Also, I think the `Ready` and `FetchFailed` status are quite confusing and we actually don't need them so they're removed in this patch. Then, we will always go to `Finished` instead. It would basically be following steps: ``` +-----------------+ | Failed to fetch | ----------+ +--------------+ +----------+ / +-----------------+ | | Fetch module | ----> | Fetching | ---+ v +--------------+ +----------+ \ +---------+ +---------------------+ | Fetched | | Advance to Finished | +---------+ +---------------------+ | ^ v | +-------------------+ | | Fetch descendants | ----- if no descendants +-------------------+ | V +----------------------+ | Fetching Descendants | +----------------------+ ``` In `Advance to Finished`, it means that module script is about to finished so it will 1. Notify all of its `ready` and `not finished` parents to finish 2. Link (instantiate) the module 3. Resolve its promise to notify owner(s) to finish  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25442 - [x] These changes do not require tests because it basically refactors the script module and should not break anything.

CYBAI · 2020-05-03T10:37:27Z

@bors-servo try=wpt

Added some more steps to avoid intermittent failures. Let's see if it works well!

bors-servo · 2020-05-03T10:37:32Z

⌛ Trying commit bcd101c with merge 686c87a...


        Auto merge of #26395 - CYBAI:refactor-module-script, r=<try>

Move away from Promise.all way and check if we need to finish manually In the previous Promise.all way, we registered a promise for every module script which means we will need to do many complex checkings like "is this top level?" and it will make us much more difficult to understand how the module script algorithm works. In the new manual checking way, we will only register promises for top level modules to notify its owner (e.g. the script element) to finish the load. So, we can understand it much more easily and would be more spec-aligned. Also, I think the `Ready` and `FetchFailed` status are quite confusing and we actually don't need them so they're removed in this patch. Then, we will always go to `Finished` instead. It would basically be following steps: ``` +-----------------+ | Failed to fetch | ----------+ +--------------+ +----------+ / +-----------------+ | | Fetch module | ----> | Fetching | ---+ v +--------------+ +----------+ \ +---------+ +---------------------+ | Fetched | | Advance to Finished | +---------+ +---------------------+ | ^ v | +-------------------+ | | Fetch descendants | ----- if no descendants +-------------------+ | V +----------------------+ | Fetching Descendants | +----------------------+ ``` In `Advance to Finished`, it means that module script is about to finished so it will 1. Notify all of its `ready` and `not finished` parents to finish 2. Link (instantiate) the module 3. Resolve its promise to notify owner(s) to finish  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25442 - [x] These changes do not require tests because it basically refactors the script module and should not break anything.

bors-servo · 2020-05-03T11:19:52Z

☀️ Test successful - status-taskcluster
State: approved= try=True

CYBAI · 2020-05-06T02:45:27Z

btw, is it worth writing the ASCII graph in PR description into script_module file as documentation comments 👀?

jdm · 2020-05-11T15:30:50Z

r? @Manishearth

Manishearth

Seems overall okay, but i'm having a hard time understanding how the resolution works: could you add more comments to the fetch modules and advance_finished_and_link functions?

also it seems like the advance_finished_and_link function, which contains a bunch of loops, is called a lot, but i'm unsure how necessary that is since it's unclear to me what the function is doing

components/script/script_module.rs

CYBAI · 2020-05-27T03:21:28Z

@bors-servo try=wpt

bors-servo · 2020-05-27T03:21:33Z

⌛ Trying commit 9651097 with merge a260b24...


        Auto merge of #26395 - CYBAI:refactor-module-script, r=<try>

Move away from Promise.all way and check if we need to finish manually In the previous Promise.all way, we registered a promise for every module script which means we will need to do many complex checkings like "is this top level?" and it will make us much more difficult to understand how the module script algorithm works. In the new manual checking way, we will only register promises for top level modules to notify its owner (e.g. the script element) to finish the load. So, we can understand it much more easily and would be more spec-aligned. Also, I think the `Ready` and `FetchFailed` status are quite confusing and we actually don't need them so they're removed in this patch. Then, we will always go to `Finished` instead. It would basically be following steps: ``` +-----------------+ | Failed to fetch | ----------+ +--------------+ +----------+ / +-----------------+ | | Fetch module | ----> | Fetching | ---+ v +--------------+ +----------+ \ +---------+ +---------------------+ | Fetched | | Advance to Finished | +---------+ +---------------------+ | ^ v | +-------------------+ | | Fetch descendants | ----- if no descendants +-------------------+ | V +----------------------+ | Fetching Descendants | +----------------------+ ``` In `Advance to Finished`, it means that module script is about to finished so it will 1. Notify all of its `ready` and `not finished` parents to finish 2. Link (instantiate) the module 3. Resolve its promise to notify owner(s) to finish  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25442 - [x] These changes do not require tests because it basically refactors the script module and should not break anything.

bors-servo · 2020-05-27T03:37:46Z

💔 Test failed - status-taskcluster

CYBAI · 2020-06-13T09:52:53Z

@bors-servo retry

intermittently webrender build failure on layout-2020 machine

bors-servo · 2020-06-13T09:52:58Z

⌛ Testing commit dc2c2c8 with merge e99a00d...


        Auto merge of #26395 - CYBAI:refactor-module-script, r=Manishearth

Move away from Promise.all way and check if we need to finish manually In the previous Promise.all way, we registered a promise for every module script which means we will need to do many complex checkings like "is this top level?" and it will make us much more difficult to understand how the module script algorithm works. In the new manual checking way, we will only register promises for top level modules to notify its owner (e.g. the script element) to finish the load. So, we can understand it much more easily and would be more spec-aligned. Also, I think the `Ready` and `FetchFailed` status are quite confusing and we actually don't need them so they're removed in this patch. Then, we will always go to `Finished` instead. It would basically be following steps: ``` +-----------------+ | Failed to fetch | ----------+ +--------------+ +----------+ / +-----------------+ | | Fetch module | ----> | Fetching | ---+ v +--------------+ +----------+ \ +---------+ +---------------------+ | Fetched | | Advance to Finished | +---------+ +---------------------+ | ^ v | +-------------------+ | | Fetch descendants | ----- if no descendants +-------------------+ | V +----------------------+ | Fetching Descendants | +----------------------+ ``` In `Advance to Finished`, it means that module script is about to finished so it will 1. Notify all of its `ready` and `not finished` parents to finish 2. Link (instantiate) the module 3. Resolve its promise to notify owner(s) to finish  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25442 - [x] These changes do not require tests because it basically refactors the script module and should not break anything.

bors-servo · 2020-06-13T10:29:37Z

💔 Test failed - status-taskcluster

CYBAI · 2020-06-13T11:09:12Z

@bors-servo retry

  ▶ Unexpected subtest result in /_webgl/conformance/textures/misc/copy-tex-image-2d-formats.html:
  │ FAIL [expected PASS] WebGL test #47: should be 64,255,191,255\nat (0, 0) expected: 64,255,191,255 was 0,0,0,255
  │   → assert_true: should be 64,255,191,255
  │   → at (0, 0) expected: 64,255,191,255 was 0,0,0,255 expected true got false
  │ 
  │ reportTestResultsToHarness/<@http://web-platform.test:8000/_webgl/js/js-test-pre.js:123:24
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1977:25
  │ test@http://web-platform.test:8000/resources/testharness.js:535:30
  │ reportTestResultsToHarness@http://web-platform.test:8000/_webgl/js/js-test-pre.js:122:15
  │ testFailed@http://web-platform.test:8000/_webgl/js/js-test-pre.js:249:31
  │ checkCanvasRect/<@http://web-platform.test:8000/_webgl/js/webgl-test-utils.js:1269:19
  │ checkCanvasRectColor@http://web-platform.test:8000/_webgl/js/webgl-test-utils.js:1225:20
  │ checkCanvasRect@http://web-platform.test:8000/_webgl/js/webgl-test-utils.js:1257:23
  │ checkCanvas@http://web-platform.test:8000/_webgl/js/webgl-test-utils.js:1287:18
  │ testCopyTexImage2D@http://web-platform.test:8000/_webgl/conformance/textures/misc/copy-tex-image-2d-formats.html:188:7
  │ testFormats@http://web-platform.test:8000/_webgl/conformance/textures/misc/copy-tex-image-2d-formats.html:133:23
  └ @http://web-platform.test:8000/_webgl/conformance/textures/misc/copy-tex-image-2d-formats.html:99:12

is this same as #26862?

bors-servo · 2020-06-13T11:25:07Z

⌛ Testing commit dc2c2c8 with merge 679f02b...


        Auto merge of #26395 - CYBAI:refactor-module-script, r=Manishearth

Move away from Promise.all way and check if we need to finish manually In the previous Promise.all way, we registered a promise for every module script which means we will need to do many complex checkings like "is this top level?" and it will make us much more difficult to understand how the module script algorithm works. In the new manual checking way, we will only register promises for top level modules to notify its owner (e.g. the script element) to finish the load. So, we can understand it much more easily and would be more spec-aligned. Also, I think the `Ready` and `FetchFailed` status are quite confusing and we actually don't need them so they're removed in this patch. Then, we will always go to `Finished` instead. It would basically be following steps: ``` +-----------------+ | Failed to fetch | ----------+ +--------------+ +----------+ / +-----------------+ | | Fetch module | ----> | Fetching | ---+ v +--------------+ +----------+ \ +---------+ +---------------------+ | Fetched | | Advance to Finished | +---------+ +---------------------+ | ^ v | +-------------------+ | | Fetch descendants | ----- if no descendants +-------------------+ | V +----------------------+ | Fetching Descendants | +----------------------+ ``` In `Advance to Finished`, it means that module script is about to finished so it will 1. Notify all of its `ready` and `not finished` parents to finish 2. Link (instantiate) the module 3. Resolve its promise to notify owner(s) to finish  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25442 - [x] These changes do not require tests because it basically refactors the script module and should not break anything.

bors-servo · 2020-06-13T11:40:59Z

💔 Test failed - status-taskcluster

CYBAI · 2020-06-13T12:01:13Z

@bors-servo retry

#26855

bors-servo · 2020-06-13T12:33:56Z

⌛ Testing commit dc2c2c8 with merge 2ce4bc5...

bors-servo · 2020-06-13T12:40:51Z

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing 2ce4bc5 to master...


        Rollback to recursively check dependency status

In servo#26395, we moved from `recursive checking` of dependency status to check only the _current module_'s dependency status and its descendant dependency status and also circular dependency status. However, it will cause an issue. For example, if the module dependency is like following ``` a -> b -> c -> d -> e f -> g -> h -> c -> d -> e ``` In this example, if the d module is still under fetching but g is trying to advance to finish. Then, it will cause a panic because module d is g's grand-grand-grand-descendant which means it's still under fetching and we can't instantiate module g. Ideally, we should get rid of the checking in servo#26903 so, before servo#26903 fixed, we can just move back to the recursive checking way which will ensure all descendants are not fetching.

highfive assigned jdm May 3, 2020

highfive added the S-awaiting-review label May 3, 2020

highfive added the S-tests-failed label May 3, 2020

CYBAI force-pushed the CYBAI:refactor-module-script branch from ca52155 to 2e8381d May 3, 2020

highfive removed the S-tests-failed label May 3, 2020

CYBAI force-pushed the CYBAI:refactor-module-script branch from 53dfd18 to bcd101c May 3, 2020

highfive assigned Manishearth and unassigned jdm May 11, 2020

Manishearth reviewed May 11, 2020

View changes

CYBAI force-pushed the CYBAI:refactor-module-script branch from bcd101c to 9651097 May 27, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 13, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 13, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 13, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 13, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 13, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 13, 2020

bors-servo merged commit 2ce4bc5 into servo:master Jun 13, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 13, 2020

CYBAI deleted the CYBAI:refactor-module-script branch Jun 13, 2020

This was referenced Jun 22, 2020

Nested imports while importing higher than 2 level modules will panic #27029

Closed

Fix nested modules while imported under more than 3 levels #27034

Merged

servo / servo

Move away from Promise.all way and check if we need to finish manually #26395

Move away from Promise.all way and check if we need to finish manually #26395

CYBAI commented May 3, 2020

highfive commented May 3, 2020

highfive commented May 3, 2020

CYBAI commented May 3, 2020

CYBAI commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

CYBAI commented May 3, 2020

bors-servo commented May 3, 2020

CYBAI commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

CYBAI commented May 6, 2020

jdm commented May 11, 2020

Manishearth left a comment

CYBAI commented May 27, 2020

bors-servo commented May 27, 2020

bors-servo commented May 27, 2020

CYBAI commented Jun 13, 2020

bors-servo commented Jun 13, 2020

bors-servo commented Jun 13, 2020

CYBAI commented Jun 13, 2020

bors-servo commented Jun 13, 2020

bors-servo commented Jun 13, 2020

CYBAI commented Jun 13, 2020

bors-servo commented Jun 13, 2020

bors-servo commented Jun 13, 2020

servo / servo

Join GitHub today

Move away from Promise.all way and check if we need to finish manually #26395

Move away from Promise.all way and check if we need to finish manually #26395

Conversation

CYBAI commented May 3, 2020

highfive commented May 3, 2020

highfive commented May 3, 2020

CYBAI commented May 3, 2020

CYBAI commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

CYBAI commented May 3, 2020

bors-servo commented May 3, 2020

CYBAI commented May 3, 2020

bors-servo commented May 3, 2020

bors-servo commented May 3, 2020

CYBAI commented May 6, 2020

jdm commented May 11, 2020

Manishearth left a comment

CYBAI commented May 27, 2020

bors-servo commented May 27, 2020

bors-servo commented May 27, 2020

CYBAI commented Jun 13, 2020

bors-servo commented Jun 13, 2020

bors-servo commented Jun 13, 2020

CYBAI commented Jun 13, 2020

bors-servo commented Jun 13, 2020

bors-servo commented Jun 13, 2020

CYBAI commented Jun 13, 2020

bors-servo commented Jun 13, 2020

bors-servo commented Jun 13, 2020