10000 Wrap "call data" with an IMEMO object by tenderlove · Pull Request #2627 · ruby/ruby · GitHub
[go: up one dir, main page]
Skip to content

Wrap "call data" with an IMEMO object #2627

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and 8000 privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Wrap "call data" with an IMEMO object #2627

wants to merge 1 commit into from

Conversation

tenderlove
Copy link
Member

Currently, compaction will invalidate all inline caches. I would like
to update the compactor to fix references in inline caches.
Unfortunately this static variable is not reachable from the GC. This
commit introduces a "call data" imemo type to wrap the call data. This
way the GC can see the static call data variable and update its
references.

@tenderlove tenderlove force-pushed the imemo-call-data branch 4 times, most recently from ace459f to ef19e51 Compare October 29, 2019 00:12
@tenderlove
Copy link
Member Author

I added the GCC part back in because I actually ruined all of the other call sites. The other call sites should be restored now.

@jhawthorn jhawthorn mentioned this pull request Oct 29, 2019
Merged
@shyouhei shyouhei requested a review from ko1 October 29, 2019 01:10
@shyouhei shyouhei mentioned this pull request Oct 29, 2019
Merged
@ko1
Copy link
Contributor
ko1 commented Oct 30, 2019

will you make iseq calldata to imemo?

@shyouhei
Copy link
Member

For the static storage allocated inside of rb_funcallv macro, the approach seems fine. There are other places where struct rb_call_data is allocated (like @ko1 mentions). They should also be taken care of.

@tenderlove
Copy link
Member Author

will you make iseq calldata to imemo?

No. I think there are too many. I only want to use this IMEMO for hard to reach call data (like static variables).

@shyouhei
Copy link
Member

Static inline cache also exists at mjit_copy_job_t::cc_entries defined in mjit_worker.c.

AFAIK there are several call data / call cache on stack as well. If your git(1) is compiled with PCRE enabled, the list of such functions can be obtained using git grep -pP '\bstruct (rb_call_cache|rb_call_data|rb_kwarg_call_data)(?=\s*[^\s*{)])'.

@tenderlove
Wrap "call data" with an IMEMO object
5d9d41a 
Currently, compaction will invalidate all inline caches.  I would like
to update the compactor to fix references in inline caches.
Unfortunately this static variable is not reachable from the GC.  This
commit introduces a "call data" imemo type to wrap the call data.  This
way the GC can see the static call data variable and update its
references.
@ko1
Copy link
Contributor
ko1 commented Nov 6, 2019

if you want to skip resetting all inline caches, all CCs should be cleared (because they assume MENTs don't move).

@tenderlove
Copy link
Member Author

Closing in favor of #2697

@ko1 I think I am able to update all CCs in #2697. Please check it out if you have time. It uses compaction count to verify the call cache is updated. I still need to add like #ifdef GC_VERIFY or something to remove compaction count from the CCs (only do verification in a check mode).

@tenderlove tenderlove closed this Nov 27, 2019
@tenderlove
Copy link
Member Author

Sorry, I mean #2698, not #2697

@tenderlove tenderlove deleted the imemo-call-data branch November 27, 2019 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ko1 ko1 Awaiting requested review from ko1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3F94
Development

Successfully merging this pull request may close these issues.
3 participants
@tenderlove @ko1 @shyouhei
0