Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a namespace installer #23934

Merged
merged 1 commit into from Aug 21, 2019
Merged

Conversation

@gterzian
Copy link
Member

gterzian commented Aug 8, 2019


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23932 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive
Copy link

highfive commented Aug 8, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs, components/script/script_thread.rs
  • @cbrewster: components/constellation/constellation.rs
  • @KiChjang: components/script/script_thread.rs
@highfive
Copy link

highfive commented Aug 8, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@gterzian
Copy link
Member Author

gterzian commented Aug 8, 2019

I think this is better than the current manual counting of pipeline-namespace that goes on in servo/lib and constellation, it becomes downright useful in the light of for example #23637

Where in order to install a pipeline-namespace in a worker thread, you need to make a blocking ipc call to the constellation, see https://github.com/servo/servo/pull/23637/files#diff-c3210c2a1bf5e1de39588154cd766100R390

The above could be replaced by a simple call in the worker to PipelineNamespace::install(ProcessNamespace::next_pipeline_namespace_id());

It would also be useful to install a pipeline-namespace in threads that do not have a direct channel to the constellation, for example the style threadpool(the reason why it might need one is anything requiring creating unique Ids, for example using the shared-ipc router prototyped at #23909, which currently can't work with parallel layout, as the communication with the font-cache occurs from a rayon thread from which it isn't easy to ask the constellation for a pipeline-namespace id).

@gterzian gterzian changed the title introduce a process namespace Introduce a process namespace Aug 8, 2019
@gterzian
Copy link
Member Author

gterzian commented Aug 8, 2019

This isn't going to work in multiprocess, since there is nothing to prevent one process to race ahead of the other if the namespace is just a u32....

I think the only way is tot actually use the constellation as a global lock...

@gterzian gterzian closed this Aug 8, 2019
@gterzian gterzian reopened this Aug 8, 2019
@gterzian
Copy link
Member Author

gterzian commented Aug 8, 2019

Actually, the last commit might be something the could work. It means we'd still use the constellation to provide the PipelineNamspaceId to install a pipeline-namespace, and it would then be shared with all threads in the process, so each could use it to create Ids without having to install it's own.

The downside is that the namespace becomes a kind of contended resource for all threads in a process, yet this might not matter in practice. If it were to become a problem, it could be solved by adding some intermediary per-thread namespace, that would be installed using the per-process namespace...

This could definitely make it much easier to start using the name-space from a bunch of other threads, without having to orchestrate installing it via the constellation.

@gterzian gterzian force-pushed the gterzian:add_process_name_space branch 3 times, most recently from 216426d to 74de1ab Aug 8, 2019
@gterzian gterzian mentioned this pull request Aug 9, 2019
0 of 5 tasks complete
@gterzian
Copy link
Member Author

gterzian commented Aug 9, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Aug 9, 2019

Trying commit be5d0de with merge d8aef35...

bors-servo added a commit that referenced this pull request Aug 9, 2019
Introduce a process namespace

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23932 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23934)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 9, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

@gterzian gterzian force-pushed the gterzian:add_process_name_space branch 5 times, most recently from 266e69b to e889353 Aug 13, 2019
@gterzian
Copy link
Member Author

gterzian commented Aug 13, 2019

@jdm Ok here is the latest variation on the same theme. I think this can be a way to address https://github.com/servo/servo/pull/23637/files#r312133197 generally across all threads.

So, still using the constellation as a single source of unique per UA PipelineNamespaceId, and now adding a per-process PipelineNameSpaceInstaller that is shared with all threads in the process, and that can be used to setup thread-local pipeline-namespaces.

It's better than the previous version since it keeps the actual namespace as a thread-local, and it improves on the current situation since it makes it easy for any thread to install a namespace.

It improves on the code at https://github.com/servo/servo/pull/23637/files by removing the need for threads to have their own plumbing to ask the constellation for a namespace. For example, with this any thread in a rayon pool could get it's own namespace without needing to setup a channel with the constellation directly or otherwise being aware of how a namespace is installed.

Still building to test if it actually works, I think it should be ok.

@gterzian gterzian force-pushed the gterzian:add_process_name_space branch 6 times, most recently from e7daa00 to 55e6ca1 Aug 13, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Aug 14, 2019

Trying commit 0630426 with merge 9910907...

bors-servo added a commit that referenced this pull request Aug 14, 2019
Introduce a namespace installer

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23932 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23934)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 14, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

@gterzian
Copy link
Member Author

gterzian commented Aug 15, 2019

@jdm this is is ready for review, I haven't squashed those commits since I'm not sure the "batching of Ids" is a good idea, so that commit could still be removed.

This is somewhat relevant for the messageport PR, since it could rebase off of this one and simplify the worker code that installs pipeline namespaces.

Copy link
Member

jdm left a comment

These changes look fine, but I don't think we need the batch generation.

@@ -818,6 +818,18 @@ where
namespace_id
}

/// Pre-generate a buffer of namespace ids, so content, or other, processes,
/// do not require to ipc for them initially.

This comment has been minimized.

Copy link
@jdm

jdm Aug 21, 2019

Member

I imagine that any IPC for pipeline namespace generation will be completely dwarfed by the time it takes for the network request to complete. I don't think this is worth including without data backing it up.

@gterzian gterzian force-pushed the gterzian:add_process_name_space branch from 684d897 to 8b98953 Aug 21, 2019
@jdm jdm removed the S-awaiting-review label Aug 21, 2019
@jdm
jdm approved these changes Aug 21, 2019
@gterzian gterzian force-pushed the gterzian:add_process_name_space branch from 8b98953 to 34008a3 Aug 21, 2019
@gterzian
Copy link
Member Author

gterzian commented Aug 21, 2019

Ok, rebased as well...

@jdm
Copy link
Member

jdm commented Aug 21, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

📌 Commit 34008a3 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

Testing commit 34008a3 with merge ba1fa08...

bors-servo added a commit that referenced this pull request Aug 21, 2019
Introduce a namespace installer

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23932 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23934)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 21, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing ba1fa08 to master...

@bors-servo bors-servo merged commit 34008a3 into servo:master Aug 21, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:add_process_name_space branch Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.