-
Notifications
You must be signed in to change notification settings - Fork 10
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
[PROF-11405] Fix managed string storage causing corrupt profiles #896
base: main
Are you sure you want to change the base?
[PROF-11405] Fix managed string storage causing corrupt profiles #896
Conversation
**What does this PR do?** This PR fixes a bug with managed string storage's cache invalidation logic that caused corrupt profiles. Specifically, profiles could be emitted that referenced wrong or even non-existing entries in the string table, which would fail to parse. **Motivation:** Fix managed string storage bug. **Additional Notes:** Prior to this PR, the managed string table was using the profile's `StringTable` pointer as a key for cache invalidation in the `ManagedStringId` to `StringId` mapping. This is not correct since when a profile gets reset, the `StringTable` pointer **doesn't change**. This bug lay dormant because Ruby uses two profiles and switches back and forth between them. This meant that in most cases the cache would go back and forth between the two profiles, working correctly... until a cache entry was left behind and got reused incorrectly. **How to test the change?** I've added a test to make sure we don't regress here.
BenchmarksComparisonBenchmark execution time: 2025-02-27 11:53:19 Comparing candidate commit 9469f89 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #896 +/- ##
==========================================
+ Coverage 71.77% 72.04% +0.26%
==========================================
Files 328 328
Lines 48702 48786 +84
==========================================
+ Hits 34955 35146 +191
+ Misses 13747 13640 -107
|
…cache-invalidation
What does this PR do?
This PR fixes a bug with managed string storage's cache invalidation logic that caused corrupt profiles.
Specifically, profiles could be emitted that referenced wrong or even non-existing entries in the string table, which would fail to parse.
Motivation
Fix managed string storage bug.
Additional Notes
Prior to this PR, the managed string table was using the profile's
StringTable
pointer as a key for cache invalidation in theManagedStringId
toStringId
mapping.This is not correct since when a profile gets reset, the
StringTable
pointer doesn't change.This bug lay dormant because Ruby uses two profiles and switches back and forth between them. This meant that in most cases the cache would go back and forth between the two profiles, working correctly... until a cache entry was left behind and got reused incorrectly.
How to test the change?
I've added a test to make sure we don't regress here.