Use concurrent set for global symbol table #13948
Open
+510
−417
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
rb_objspace_garbage_object_p expects only GC managed objects to be passed in. We should skip the check if curr_key is a special constant.
We don't need to delay the freeing of the fstr for the symbol if we store the hash of the fstr in the dynamic symbol and we use compare-by-identity for removing the dynamic symbol from the sym_set.
If the object is garbage, then calling cmp on it may crash.
Benchmark:
ARGV[0].to_i.times.map do
Ractor.new do
1_000_000.times do |i|
"hello#{i}".to_sym
end
end
end.map(&:value)
Results:
| Ractor count | Branch (s) | Master (s) |
|--------------|------------|------------|
| 1 | 0.364 | 0.401 |
| 2 | 0.555 | 1.149 |
| 3 | 0.583 | 3.890 |
| 4 | 0.680 | 3.288 |
| 5 | 0.789 | 5.107 |
If we create a key but don't insert it (due to other Ractor winning the race), then it would leak memory if we don't free it. This introduces a new function to free that memory for this case.
❌ Tests Failed✖️no tests failed ✔️62102 tests passed(1 flake) |
jhawthorn
reviewed
Jul 18, 2025
concurrent_set.c
Outdated
| struct concurrent_set *set = RTYPEDDATA_GET_DATA(set_obj); | ||
|
|
||
| struct concurrent_set_probe probe; | ||
| VALUE hash = set->funcs->hash(key); |
There was a problem hiding this comment.
(non-blocking) I doubt it makes a significant difference, but I think hash won't change and could go above retry:
There was a problem hiding this comment.
Good catch 👍 I copied the code from rb_concurrent_set_find_or_insert which also has this issue so I fixed it in both places.
There was a problem hiding this comment.
Ah, I see. That didn't work because we only load set later (even though we know a new set will still have the same funcs).
I don't think this change is worth making 😅
Since the hash should never change, we can calculate it once before the retry.
jhawthorn
approved these changes
Jul 19, 2025
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 PR switches the global symbol table and allows for symbol lookup and creation of dynamic symbols without needing the global VM lock. Creating static symbols still require the global VM lock but I will be investigating how to make that lock-free in the future.
Here's a microbenchmark demonstrating the speedup:
Results:
Raw results: