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 ProcessNamespace? #23932

Closed
gterzian opened this issue Aug 8, 2019 · 2 comments
Closed

Introduce ProcessNamespace? #23932

gterzian opened this issue Aug 8, 2019 · 2 comments

Comments

@gterzian
Copy link
Member

@gterzian gterzian commented Aug 8, 2019

In both #23637 and #23909 I find myself installing PipelineNamspace in various threads that currently do not have one, mainly as a way to use various new types of Id that aren't PipelineId, like MessagePortId or IpcRouterId. Those need to follow a similar model since they can end-up crossing process boundaries(message-port), or need to be unique per process.

It can be quite hard to install a pipeline namespace for some threads, especially if those threads do not have a direct channel to the constellation. For example the threadpool used by layout, from within which calls are made to the font-cache(which I tried to refactor to use this prototype SharedIpcRouter).

For other threads, like dedicated workers and layout, it's merely an inconvenient blocking ipc call to the constellation.

So, perhaps we could have a ProcessNamespace be a lazy static, put a mutex around it(or make it an atomic), then each thread upon initialization could grab it, install a thread-local PipelineNamespace based on something like ProcessNamespace::new_pipeline_id(), which could be an easy way for a thread to install a PipelineNamespace without having to communicate with the constellation.

And the ProcessNamespace is something the constellation could seed, like is currently done with PipelineNamespace, when the process is created.

@gterzian gterzian changed the title Improve `PipelineNamespace`, Introduce ProcessNamespace? Improve `PipelineNamespace`, Introduce ProcessId? Aug 8, 2019
@gterzian gterzian changed the title Improve `PipelineNamespace`, Introduce ProcessId? Improve `PipelineNamespace`, Introduce ProcessNamespace? Aug 8, 2019
@gterzian gterzian changed the title Improve `PipelineNamespace`, Introduce ProcessNamespace? Introduce ProcessNamespace? Aug 8, 2019
@gterzian gterzian mentioned this issue Aug 8, 2019
0 of 5 tasks complete
@gterzian
Copy link
Member Author

@gterzian gterzian commented Aug 8, 2019

I've tried, it felt like a good idea for a few hours, then I realized a process could just race ahead of another, and the only way to make sure each process is creating unique IDs is literally by using the constellation as a global lock and the single source of increasing namespace ids...

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

@gterzian gterzian commented Aug 8, 2019

However, this might still work if we were to make the PIPELINE_NAMESPACE itself something that is shared across threads. So that still requires the constellation to provide the PipelineNamespaceId, but that needs to be done only once per process, if we share the installed namespace with all threads in the process.

bors-servo added a commit that referenced this issue 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 added a commit that referenced this issue Aug 13, 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 added a commit that referenced this issue 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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

1 participant
You can’t perform that action at this time.