-
Notifications
You must be signed in to change notification settings - Fork 884
Optimize resume of non-suspending continuations #9071
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
Merged
+1,196
−0
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
9a36dad
Add a suspends effect
tlively f3f4698
Optimize resume of non-suspending continuations
tlively 64465af
address feedback
tlively 75fa6e3
Merge branch 'suspends-effect' into directize-resume
tlively cb44856
disable stack switching on affected tests
tlively d1dedd4
Merge branch 'suspends-effect' into directize-resume
tlively de31b49
update binaryenjs test
tlively 114fa8e
Merge branch 'suspends-effect' into directize-resume
tlively e64929f
more test comments
tlively 7daad92
Merge branch 'suspends-effect' into directize-resume
tlively a3ffe83
use ChildLocalizer
tlively c0f49ff
Merge remote-tracking branch 'origin/main' into directize-resume
tlively 62a1135
Merge branch 'main' into directize-resume
tlively d08bc58
look through tee on ref.func
tlively ba9c96c
update tests
tlively File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that this is almost but not quite right. ChildLocalizer considers child effects wrt each other. After the operation, it is safe to reorder and remove them. But moving them across other effects might not be safe.
Unless we know no other effects can be in the middle, here? But it seems like there can be:
OPERANDSis moved past theSTUFFs.This is simple to handle, though: add a flag to ChildLocalizer to control this behavior. Where it computes
then, we can have a mode where it uses a local for any child with effects, even removable ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But
ChildLocalizersees all the children of theresume, including the one that hasSTUFF AandSTUFF Bhere, so I think this is safe. See the$test-eval-ordertest, for example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right... all the last child (with STUFF) is, is just another child for ChildLocalizer. Nice it works out so well!