Skip to content

Make bound method management threadsafe#7917

Merged
headius merged 1 commit intojruby:jruby-9.3from
chadlwilson:bound-methods-thread-safety
Sep 4, 2023
Merged

Make bound method management threadsafe#7917
headius merged 1 commit intojruby:jruby-9.3from
chadlwilson:bound-methods-thread-safety

Conversation

@chadlwilson
Copy link
Contributor

@chadlwilson chadlwilson commented Aug 31, 2023

I'm not quite sure the appropriate sizing of the Maps here, but tried to follow existing conventions elsewhere and be conscious of the additional overhead of ConcurrentHashMaps for the nested Maps. But this may also be premature optimization in some areas, so will take maintainer guidance.

  • Kept the second level maps initial size small by default (maybe too small?)
  • Used 0.9 load factors similar to those in RubyModule
  • We shouldn't need high concurrency here, so set to 2. Even 1 might be fine?

I also can't find an appropriate place to write particular tests here, so any pointers useful.

Needs to be ported forward to 9.4/master if suitable.

@headius
Copy link
Member

headius commented Sep 4, 2023

Could you actually just rebase this on 9.3? We are still merging 9.3 changes forward to 9.4 and this should be fixed in both.

It looks good to me! Thanks!

@headius headius added this to the JRuby 9.3.11.0 milestone Sep 4, 2023
Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@chadlwilson chadlwilson force-pushed the bound-methods-thread-safety branch from 6e674a1 to 323aa6e Compare September 4, 2023 08:45
@chadlwilson chadlwilson changed the base branch from master to jruby-9.3 September 4, 2023 08:45
@chadlwilson
Copy link
Contributor Author

@headius oh okay, done now. I thought you used a backport approach rather than merge forward. My bad.

@headius
Copy link
Member

headius commented Sep 4, 2023

Looks good!

@headius headius merged commit 8ccff43 into jruby:jruby-9.3 Sep 4, 2023
@chadlwilson chadlwilson deleted the bound-methods-thread-safety branch September 4, 2023 09:21
@chadlwilson
Copy link
Contributor Author

Do you want me to open a cherry-pick PR to master, or do you have a regular process to do this?

@headius
Copy link
Member

headius commented Sep 4, 2023

Nope, already merged! We find it easier to merge forward because most changes happen on master, and everything fixed in a maintenance branch should also be applied to master.

@headius
Copy link
Member

headius commented Sep 4, 2023

@enebo Another one we should get 9.3.11 out for soon...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ConcurrentModificationException during RubyModule.defineAnnotatedMethods / Ruby.addBoundMethod

2 participants