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

New flag --profile replaces verbosity option -vprofile #5781

Closed
nad opened this issue Feb 8, 2022 · 4 comments · Fixed by #5796
Closed

New flag --profile replaces verbosity option -vprofile #5781

nad opened this issue Feb 8, 2022 · 4 comments · Fixed by #5796
Assignees
Labels
documented-in-changelog Issues already documented in the CHANGELOG type: bug Issues and pull requests about actual bugs
Milestone

Comments

@nad
Copy link
Contributor

nad commented Feb 8, 2022

I noticed that -vprofile:7 overrides -vprofile.definitions:10 and -vprofile.modules:10:

return $ case (internal, defs, modules) of
(True, _, _) -> B.BenchmarkSome isInternalAccount
(_, True, _) -> B.BenchmarkSome isDefAccount
(_, _, True) -> B.BenchmarkSome isModuleAccount
_ -> B.BenchmarkOff

Is this intentional? It is not documented.

@nad nad added the type: bug Issues and pull requests about actual bugs label Feb 8, 2022
@nad nad added this to the 2.6.3 milestone Feb 8, 2022
@nad nad self-assigned this Feb 9, 2022
@nad
Copy link
Contributor Author

nad commented Feb 9, 2022

I guess the reason is that the current implementation cannot handle several of these options at once: if you activate both -vprofile:7 and -vprofile.modules:10 (say), then the output for modules will be incorrect, because only the innermost call to billTo "counts".

I see some ways forward:

  • Make the code work.
  • Make Agda complain if two or more of these options are activated at the same time. However, that would mean that Agda complains if you use -vprofile:10. An alternative is to use dedicated options for profiling.
  • Update the documentation.

Given that I think this bug has low priority I don't want to spend too much time on it. I suggest that we simply update the documentation.

@nad nad removed their assignment Feb 9, 2022
@andreasabel
Copy link
Member

The usual semantics if -v is cumulative, so more instances of this flag can only add more output.

@UlfNorell
Copy link
Member

The usual semantics if -v is cumulative, so more instances of this flag can should only add more output.

Indeed, the use of -v for profiling is a bit of a hack, and we really should have a separate flag for it. How about

--profile=internal|definitions|modules|...

Aside from those three we also have sharing, serialize, metas, constraints, interactive, iface, and ticks. I believe those aren't using the billing system so they can all coexist without problems.

@nad
Copy link
Contributor Author

nad commented Feb 11, 2022

Sounds good to me.

@andreasabel andreasabel changed the title No information about modules in output for -vprofile.modules:10 New flag --profile replaces verbosity option -vprofile Oct 24, 2022
@nad nad added the documented-in-changelog Issues already documented in the CHANGELOG label Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documented-in-changelog Issues already documented in the CHANGELOG type: bug Issues and pull requests about actual bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants