revert: multiprocess kind processing #746
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While this worked very well on Linux, it causes issues anywhere we can't `fork` to get a new process (most notably on Windows). The problem lies in the fact that in these cases, we spawn an entire new process, which re-imports taskgraph from scratch. This is fine in some cases, but in any case where global state has been modified in an earlier part of `TaskGraphGenerator._run`, we lose whatever side effects happened there, and end up failing in some way. Concretely: in gecko we add a bunch of `payload_builders` as part of registering the graph config. This code doesn't re-run in the spawned processes, so the payload builders don't exist there. There are workarounds for this: for example, redoing all the earlier work of `_run` in each subprocess, or perhaps finding some way to ensure all the needed state is passed explicitly. There's no quick and easy way to make this work though, and some thought should be given to the tradeoffs of doing it (vs. doing nothing, or spending the effort on a different way to parallelize) before proceeding.
Contributor
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While this worked very well on Linux, it causes issues anywhere we can't
forkto get a new process (most notably on Windows). The problem lies in the fact that in these cases, we spawn an entire new process, which re-imports taskgraph from scratch. This is fine in some cases, but in any case where global state has been modified in an earlier part ofTaskGraphGenerator._run, we lose whatever side effects happened there, and end up failing in some way.Concretely: in gecko we add a bunch of
payload_buildersas part of registering the graph config. This code doesn't re-run in the spawned processes, so the payload builders don't exist there.There are workarounds for this: for example, redoing all the earlier work of
_runin each subprocess, or perhaps finding some way to ensure all the needed state is passed explicitly. There's no quick and easy way to make this work though, and some thought should be given to the tradeoffs of doing it (vs. doing nothing, or spending the effort on a different way to parallelize) before proceeding.