[IGNORE] PR 5719#5793
Conversation
vpellan
commented
May 20, 2026
|
👋 Hey @DataDog/ruby-guild, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md(possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None.(possible answers No/Nope/None) Visited at: 2026-05-20 13:39:38 UTC |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 6fe7cc9 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-05-21 09:59:35 Comparing candidate commit 6fe7cc9 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 0 unstable metrics.
|
| # We don't quite understand yet _why_ the crash happens, but using the `object_id` instead of the object | ||
| # itself seems to work around whatever's going wrong inside the VM. | ||
| def resolve(value) | ||
| cache_key = value.object_id |
There was a problem hiding this comment.
One issue with this is, unless the return value of super below is guaranteed to hold onto value then the cache won't hold onto value at all. And that means, value can GC while still "being part of the cache semantically". Which then could mean another object could use the same object_id and the cache would return incorrect results.
It seems in Ruby 3.2+ object_id is an increasing number so probably it wouldn't get reused until fully wrapping around:
$ ruby -ve '10.times { p Object.new.object_id }'
ruby 3.2.11 (2026-03-27 revision 5483bfc1ae) [arm64-darwin25]
60
80
100
120
140
160
180
200
220
240
It still feels rather brittle to me.
Also getting the object_id is actually an extra hash lookup on 3.2: https://github.com/ruby/ruby/blob/ruby_3_2/gc.c#L4767
That's fixed to be an object field on master, but either way it's also gonna cause some extra memory usage and shape polymorphism (shapes with and without object_id will be seen at given instance variable accesses in source code, which makes such code slower, especially in the interpreter which only has a monomorphic cache).
There was a problem hiding this comment.
Ah I should have posted on #5719 probably, where a similar concern has been raised.
There was a problem hiding this comment.
(We could change the cache to hold on to a pair [original key, actual value], but I've been chatting with @vpellan that after we "stop the bleeding" we really need to do is understand a bit more what's being cached exactly and why, since we're handwaving a lot around this code and I think we need to get back to being confident we exactly know why it's needed and what's it's doing)