Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upInvestigate if we can move away from Promise.all way for module script #25442
Assignees
Labels
Comments
|
Just did some investigation today. And yes, I think we can move away from the Promise.all way. We should be able to run fetch the descendants of and link a module script out of With moving that logic out of But, we might still need a promise to let us finish the module owner (e.g. run Will update or send a draft PR if I found anything else! |
bors-servo
added a commit
that referenced
this issue
May 3, 2020
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
<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
May 3, 2020
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
<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
May 3, 2020
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
<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
May 27, 2020
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
<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
May 27, 2020
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
<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Jun 13, 2020
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
<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Jun 13, 2020
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
<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Jun 13, 2020
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
<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, we save a
promisein each ModuleTree and then use it to do things asynchronously. When we're going to fetch descendants for a module, we will merge all the descendant promises with aPromise.allso that we can do something after all descendants fetched.But, maybe we can do it better with just manually handling them with tasks and the _Trusted_s?
Need to investigate that.
Also, not sure if we can avoid using the recursive way for checking descendants after switching to use tasks🤔
Also see https://mozilla.logbot.info/servo/20200106#c16838760