pg: Add ScopeName -> Vec<GroupName> mapping

[leonqadirie]

Supersedes #189 and introduces an index field to the PgState struct to map ScopeName to Vec<GroupName>.

If this isn't what was in mind, I'd need another pointer :)
Would also be open to refactoring the index's value and the modules Vec<...>-method returns to HashSets if that makes more sense.

And sorry for the PR noise :)

--
Relating to #177 (comment)

This is where having another index might be helpful, i.e. Scope -> Group name in a separate mapping. But that's an optimization that can be added in the future.

and #184 (comment)

I meant another global index linking the scopes to groups. So you could efficiently lookup the groups from a given scope. This would remove some scan operations but it is a minimal operation.

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (8c310e7) 79.73% compared to head (95ec939) 79.75%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
ractor/src/pg/mod.rs	92.45%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   79.73%   79.75%   +0.01%     
==========================================
  Files          50       50              
  Lines        9790     9824      +34     
==========================================
+ Hits         7806     7835      +29     
- Misses       1984     1989       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[slawlor]

so the problem that this approach takes (which is why it wasn't done this way in the beginning) is that this is technically a subtle race condition. We will probably need a global lock around both of the hash maps or something, but then there's going to be the benchmarking of the performance hit for taking a global lock.

Dashmaps are sharded maps so they can do concurrent updates from multiple threads, but what we'd be doing is locking around both maps with a singleton global lock.

Unless we can try and acquire both sharded locks, doing the updates, and then only releasing the locks simultaneously.

i.e.

lock(a)
lock(b)
update(a)
update(b)
release(a)
release(b)

It will be a little ugly with the scopes, but it's the best option for concurrent updates.

[leonqadirie]

You're right of course.
I hope to find the time to experiment and benchmark, can't currently promise a timeline, though :)

[leonqadirie]

Then again, as far as I understand we are only talking about the map and index fields which are only mutated in the join* and leave* functions.

It duplicates a bit more code but I think just locking both fields and then nesting their updates (so the locks get dropped simultaneously when leaving scope) might work.

[slawlor]

One small change left and I think we're good to merge it! Thanks for all the work put into this PR!

[slawlor]

so to the point above, i think we'd move this before the removal of the index? I still need to think a bit if there's a race that could occur here even with the drop operations release the locks sequentially

[slawlor]

maybe to be safe, move this outside of the if statement on line 224. That way we check / update the index regardless of what the occupied map says

[leonqadirie]

Thanks for all the work put into this PR!

It was/is a(n educative) pleasure. I'm learning quite a lot from this code base and your reviews.

[slawlor]

LGTM, let's merge it!