Skip to content

YJIT: limit size of call count stats dict #10858

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 privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 28, 2024
Merged

YJIT: limit size of call count stats dict #10858

merged 2 commits into from
May 28, 2024

Conversation

maximecb
Copy link
Contributor

@maximecb maximecb commented May 28, 2024 •
edited
Loading

Someone reported two weeks ago that logs were getting bloated because the ISEQ and C call count dicts were huge, since they include all of the call sites. I wrote code on the Rust size to limit the size of the dict to avoid this problem. The size limit is hardcoded at 20, but I figure this is probably fine?

@maximecb
YJIT: limit size of call count stats dict
6864c0f 
Someone reported that logs were getting bloated because the
ISEQ and C call count dicts were huge, since they include all
of the call sites. I wrote code on the Rust size to limit the
size of the dict to avoid this problem. The size limit is
hardcoded at 20, but I figure this is probably fine?
@maximecb maximecb self-assigned this May 28, 2024
@matzbot matzbot requested a review from a team May 28, 2024 15:36
k0kubun
k0kubun reviewed May 28, 2024
View reviewed changes
@ruby ruby deleted a comment from launchable-app bot May 28, 2024
@maximecb maximecb merged commit 1eff5a9 into ruby:master May 28, 2024
100 checks passed
@maximecb maximecb deleted the yjit_cap_calls_dict branch May 28, 2024 17:23
@k0kubun
Copy link
Member

k0kubun commented May 28, 2024 •
edited
Loading

This would break the calculation of top_n_pct.

ruby/yjit.rb

Lines 420 to 421 in 1eff5a9

top_n_total = pairs.sum { |name, count| count }
top_n_pct = 100.0 * top_n_total / num_calls
Is that intentional?

@maximecb
Copy link
Contributor Author

This would break the calculation of top_n_pct.

ruby/yjit.rb

Lines 420 to 421 in 1eff5a9

top_n_total = pairs.sum { |name, count| count }
top_n_pct = 100.0 * top_n_total / num_calls

Is that intentional?

AFAIK it doesn't break top_n_pct, because num_calls is not calculated based on the dict, we're gathering that count separately?

@k0kubun
Copy link
Member

k0kubun commented May 28, 2024

🤦 You're right, I was reading it wrong. I'm sorry.

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

@k0kubun k0kubun k0kubun left review comments

Assignees

@maximecb maximecb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maximecb @k0kubun