Clean up frame->work lifetime #487
Merged
Conversation
This file contains 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
timo
reviewed
Jan 7, 2017
| if (frame->continuation_tags) | ||
| MVM_continuation_free_tags(tc, frame); | ||
| if (frame->env) |
There was a problem hiding this comment.
could make the diff a tiny bit smaller (easier to digest?) by moving this back up before the continuation_free_tags part.
These meant that we could not rely on ->work no longer being needed after the frame's execution was completed. This implied that, short of static analysis to check these ops were not used, we could not be sure it was OK to throw out ->work right after the frame's dynamic scope ended. The ops were not being used in Rakudo, nor in NQP besides in the code that compiled into them, which was already been removed. They only existed to allow lowering of lexicalref => localref at some point in the future, but an optimization that pessimizes the world is hardly a win. It would also have been a huge pain for the JVM backend, and likely the JS backend too, to implement localref (neither did so).
If the frame is on the call stack, we don't need to NULL stuff in it; it's going away anyway. We also eliminate a duplicate call to the function MVM_args_proc_cleanup_for_cache for the callstack case (the MVM_args_proc_cleanup in MVM_frame_destory calls it anyway). We may have continuation tags in a frame that was never promoted, since we may add tags but never actually take a continuation (a `gather` that never does `take` would be such an example), so we take care not to leak those. That duplication can be cleaned up a little later.
Unused by anything in Rakudo, and in NQP only used in a test case. It was added because nqp-j had the op, and MoarVM continuations were done to the same API. However, we never used it besides that test case, and it didn't really work properly anyway because we didn't need it, so it is a bit hard to imagine any unknown users seriously relying on it. The removal of this will help to simplify ->work lifetime management.
One-shot is all that Rakudo needs for gather/take, and will suffice for non-blocking `await` also. We never really properly handled the multi-shot case anyway, out of not needing it. And our incomplete attempts to support it came with some nasty memory management risks. The only non-trivial test for this in the NQP test suite already was failing, so there's no loss there. This protection will also be useful for catching bugs when we introduce non-blocking `await` in Rakudo, as any double invocation there will surely be a screw-up.
We no longer need the `in_continuation` flag, which eliminates the code to set it, the storage (though due to alignment we won't really save anything) and the code to check it. It was checked every time a callframe was taken off the stack, which adds up over a program's lifetime. Furthermore, we can now unconditionally clear up any continuation tags.
This is safe to do unconditionally now that continuations are one-shot and register references are no more. This brings forward when memory can be released, and also prevents things from lingering in the inter-generational set for so long (we now really only ever do that when the frame is still in dynamic scope - for very long-lived and heap-promoted frames - since we don't enforce write barriers on registers).
It now reliably provides this information.
We used to use it to track whether a frame is in dynamic scope, but now the `work` field can be reliably used for that. This shaves a memory write off every frame setup, but more significantly cuts the size of every MVMFrame by 4 (32-bit) or 8 (64-bit) bytes, which will reduce memory pressure.
Reduce to one function, removing the confusingly-named "for_cache" form that did only part of the work.
If the frame is no longer in dynamic socpe, we can't possibly have any args to care about.
Thanks to GC using this to guard against all scanning of things that are only alive in dynamic scope, there's no reason to also NULL out cur_args_callsite.
We never look at this number to decide anything except how much memory to free, which only happens once.
The ->work flag prevents us ever looking at these in GC after the lifetime of a frame is over, so this isn't needed.
Since if the frame was taken as part of a continuation that was then never invoked, its work needs cleaning up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This work ensures that the lifetime of
->work(a frame's working space) is strictly that of its lifetime on the call stack, modulo the case where the frame is not on the stack but its working storage lives on due to it having been captured in a continuation.To achieve this, two VM features unused by Perl 6 have been eliminated:
localrefwhich was also unused and never implemented by any other backend; this will likely be a relief to those implementing said other backends)With the lifetime clarified, it was possible to simplify a number of code paths, some of them hot. This also means memory can be freed sooner, reducing overhead and memory pressure.
This work leads to around 20% less time and 10% less memory in the CORE.setting build.