[RemoteMirror] Fix demangle node corruption caused by excessively eager clearing of the NodeFactory. #64674
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
|
@swift-ci please test |
|
@swift-ci please test macos platform |
al45tair
approved these changes
Mar 29, 2023
| @@ -93,6 +93,7 @@ RemoteRef<char> TypeRefBuilder::readTypeRef(uint64_t remoteAddr) { | |||
| /// Load and normalize a mangled name so it can be matched with string equality. | |||
| llvm::Optional<std::string> | |||
| TypeRefBuilder::normalizeReflectionName(RemoteRef<char> reflectionName) { | |||
| auto checkpoint = pushNodeFactoryCheckpoint(); | |||
There was a problem hiding this comment.
A lot of code that does this seems to follow the pattern
auto checkpoint = pushNodeFactoryCheckpoint();
...
popNodeFactoryCheckpoint(checkpoint);
which makes me wonder whether we should have a
template <typename T, typename F>
T withScopedNodes(F func) {
auto checkpoint = pushNodeFactoryCheckpoint();
T result = func();
popNodeFactoryCheckpoint(checkpoint);
return result;
}
or similar (obviously that's written OTOH in a Github comment, but you get the idea).
There was a problem hiding this comment.
RAII would be more idiomatic in C++. I've gone ahead and added a little RAII wrapper and switched over to that. The scoping makes things less convenient in a couple of places where we declare locals that get used later, but overall it's nicer.
There was a problem hiding this comment.
:-) I seems to have Swift on the brain at the moment. Yes, RAII is more idiomatic in C++.
mikeash
force-pushed
the
remotemirror-nodefactory-clear-fix
branch
from
9f18a44 to
4fda3b4
Compare
|
@swift-ci please test |
…er clearing of the NodeFactory. We clear the NodeFactory to prevent unbounded buildup of allocated memory, but this is done too eagerly. In particular, normalizeReflectionName can end up clearing the factory while the calling code is still using nodes that were allocated from it. To keep peak memory usage low while avoiding this problem, we introduce a checkpoint mechanism in NodeFactory. A checkpoint can be pushed and then subsequently popped. When a checkpoint is popped, only the nodes allocated since the checkpoint was pushed are invalidated and the memory reclaimed. This allows us to quickly clear short-lived nodes like those created in normalizeReflectionName, while preserving longer-lived nodes used in code calling it. Uses of clearNodeFactory are replaced with this checkpoint mechanism. rdar://106547092
mikeash
force-pushed
the
remotemirror-nodefactory-clear-fix
branch
from
4fda3b4 to
fbc3f69
Compare
|
@swift-ci please test |
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.
We clear the NodeFactory to prevent unbounded buildup of allocated memory, but this is done too eagerly. In particular, normalizeReflectionName can end up clearing the factory while the calling code is still using nodes that were allocated from it.
To keep peak memory usage low while avoiding this problem, we introduce a checkpoint mechanism in NodeFactory. A checkpoint can be pushed and then subsequently popped. When a checkpoint is popped, only the nodes allocated since the checkpoint was pushed are invalidated and the memory reclaimed. This allows us to quickly clear short-lived nodes like those created in normalizeReflectionName, while preserving longer-lived nodes used in code calling it. Uses of clearNodeFactory are replaced with this checkpoint mechanism.
rdar://106547092