-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[cDAC] Enable cache fallback on RuntimeTypeSystem and ExecutionManager contracts #132258
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
Closed
+134
−90
Closed
Changes from all commits
Commits
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.
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.
We should make a decision whether we expect CodeBlockHandle (or other XyzHandle types) to remain valid across a flush operation. I'd propose no, all handles should become invalid on Flush. If the caller expected that there was still code in the same location before and after a flush then they would need to preserve the address or whatever other identity information created the handle and call into cDAC to re-create the handle (which would also repopulate any Dictionary backed cache at the same time).
This probably means instead of trying to repopulate the handle on the fly we should go modify other handles so they don't try to auto-repopulate if they are currently doing so. We could also add a flush cookie that updates on every Flush() call so that handles can be tagged with the cookie that was active when they were created and rejected if a caller tries to reuse a stale handle.
Uh oh!
There was an error while loading. Please reload this page.
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.
The question of whether we "expect" a CodeBlockHandle or other handle to stay valid depends on what we mean by "expect". We cannot prove yes or no without invoking more complex logic; we have to know where the object was allocated from, whether we are doing time-travel debugging, etc. However, I would assume it probably does more often than not, and that the same holds for the RuntimeTypeSystem caches.
The repopulation is a best-effort attempt at driving each individual DacDbi API to completion in the case that it is still valid. If it is no longer valid, then we return an error or invalid data, and that is a bug on the caller's side (for example in the DBI). We can wrap any error in the repopulation in a more descriptive error message. But at that point, it does not really matter what we do.
This is not intended to replace the process or stop-go locks that synchronize the DBI, more as a fail-safe to mostly produce a correct answer despite any locking bugs.