Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use me->def instead of me for opt_table #4493

Merged
merged 2 commits into from
Jul 28, 2021
Merged

use me->def instead of me for opt_table #4493

merged 2 commits into from
Jul 28, 2021

Conversation

ko1
Copy link
Contributor

@ko1 ko1 commented May 12, 2021

vm_opt_method_table is a me=>bop table to manage the optimized
methods (by specialized instruction). However, me can be invalidated
to invalidate the method cache entry.
[Bug #17725]

To solve the issue, use me-def instead of me which simply copied
at invalidation timing.

A test by @jeremyevans #4376

@jeremyevans
Copy link
Contributor

@ko1 I know you wanted to use this approach instead of #4376. However, this approach doesn't appear to handle all optimized methods correctly. Is this something you are still working on? Would you be opposed to merging #4376 now, and then switching to this approach later if the issues with this approach are fixed?

ko1 added 2 commits July 28, 2021 13:40
`vm_opt_method_table` is me=>bop table to manage the optimized
methods (by specialized instruction). However, `me` can be invalidated
to invalidate the method cache entry.
[Bug #17725]

To solve the issue, use `me-def` instead of `me` which simply copied
at invalidation timing.

A test by @jeremyevans ruby#4376
Because the key of redefine table is `def`, `def` should be
unique for each optimized method (`alias` is not allowed).
@ko1
Copy link
Contributor Author

ko1 commented Jul 28, 2021

However, this approach doesn't appear to handle all optimized methods correctly.

@jeremyevans Now tests seems passes. Do you have any other issues?

@jeremyevans
Copy link
Contributor

I have no issues with this now that the tests pass. It's definitely simpler than the approach I used, so it makes sense to use this approach instead. I'll close #4376.

@ko1
Copy link
Contributor Author

ko1 commented Jul 28, 2021

Thank you for confirmation!

@ko1 ko1 merged commit fa0279d into ruby:master Jul 28, 2021
@ko1 ko1 deleted the fix_17725 branch July 28, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants