- Notifications
You must be signed in to change notification settings - Fork5.4k
Use concurrent set for global symbol table#13948
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
rb_objspace_garbage_object_p expects only GC managed objects to be passedin. 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 storethe hash of the fstr in the dynamic symbol and we use compare-by-identityfor 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 therace), then it would leak memory if we don't free it. This introduces anew function to free that memory for this case.
❌Tests Failed✖️no tests failed ✔️62102 tests passed(1 flake) |
concurrent_set.c Outdated
| structconcurrent_set*set=RTYPEDDATA_GET_DATA(set_obj); | ||
| structconcurrent_set_probeprobe; | ||
| VALUEhash=set->funcs->hash(key); |
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.
(non-blocking) I doubt it makes a significant difference, but I thinkhash won't change and could go aboveretry:
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.
Good catch 👍 I copied the code fromrb_concurrent_set_find_or_insert which also has this issue so I fixed it in both places.
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.
Ah, I see. That didn't work because we only loadset 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 theretry.
Uh oh!
There was an error while loading.Please reload this page.
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: