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

common: introduce the lower limit parameter #5909

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

janekmi
Copy link
Contributor

@janekmi janekmi commented Nov 9, 2023

This change is Reviewable

@janekmi janekmi added sprint goal This pull request is part of the ongoing sprint no changelog Add to skip the changelog check on your pull request labels Nov 9, 2023
@janekmi janekmi requested review from grom72 and osalyk November 9, 2023 15:43
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #5909 (4a06d9d) into master (f053089) will increase coverage by 1.98%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

❗ Current head 4a06d9d differs from pull request most recent head bb135f2. Consider uploading reports for the commit bb135f2 to get more accurate results

@@            Coverage Diff             @@
##           master    #5909      +/-   ##
==========================================
+ Coverage   69.02%   71.01%   +1.98%     
==========================================
  Files         131      131              
  Lines       19668    19172     -496     
  Branches     3257     3193      -64     
==========================================
+ Hits        13576    13615      +39     
+ Misses       6092     5557     -535     

@janekmi janekmi force-pushed the call_stacks-lower-limit branch from 031cf99 to 5cf12c6 Compare November 9, 2023 16:27
Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)


utils/call_stacks_analysis/generate_call_stacks.py line 25 at r1 (raw file):

PARSER.add_argument('-w', '--white-list') # XXX
PARSER.add_argument('-d', '--dump', action='store_true', help='Dump debug files')
PARSER.add_argument('-l', '--lower-limit', type=int, default=0,

Let's start using it ASAP to avoid the generation of a huge output.

Suggestion:

PARSER.add_argument('-l', '--lower-limit', type=int, default=128,

utils/call_stacks_analysis/generate_call_stacks.py line 327 at r1 (raw file):

                # are needed when the user wants to list all API calls making
                # use of a particular function in their call stacks.
                dump(small_enough, 'call_stacks_excluded', True)

Suggestion:

 dump(small_enough, 'call_stacks_below_limit', True)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi force-pushed the call_stacks-lower-limit branch from 5cf12c6 to eb8629f Compare November 10, 2023 15:16
Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @grom72 and @osalyk)


utils/call_stacks_analysis/generate_call_stacks.py line 25 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Let's start using it ASAP to avoid the generation of a huge output.

This limit reduces the final number of call stacks by 0.2%. 🤣
I think we have to do better. Find a reasonable value and put it as default.

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi force-pushed the call_stacks-lower-limit branch from eb8629f to bb135f2 Compare November 10, 2023 15:35
Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @grom72 and @osalyk)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi merged commit 00169a9 into pmem:master Nov 10, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Add to skip the changelog check on your pull request sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants