-
-
Notifications
You must be signed in to change notification settings - Fork 147
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
add threshold config parameter for FactoryProf #294
add threshold config parameter for FactoryProf #294
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Left a comment regarding the current logic. Let's fix it.
expect(output).to include("FactoryProf enabled (simple mode)") | ||
|
||
expect(output).to include("Factories usage") | ||
expect(output).to match(/Total: 16\n\s+Total top-level: 8\n\s+Total time: \d{2}+:\d{2}\.\d{3} \(out of \d{2}+:\d{2}\.\d{3}\)\n\s+Total uniq factories: 1/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only hide less used factories from the output but totals still must show the actual data, i.e., here we should see "Total uniq factories: 2".
Thus, we need to update the exclusion logic (no select!
, just skip when printing) and update the test to check that there is not match for skipped factories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfrect 👍 Thanks!
What is the purpose of this pull request?
Add threshold config parameter for
FactoryProf
to ignore factories which total number of calls is less than the provided thresholdIs there anything you'd like reviewers to focus on?
I'm not sure if support for this option is needed for flamegraph mode? As of now, I have only added support for simple mode
Checklist
Closes #290