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

Add @ballocations macro for getting number of allocations #292

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nickrobinson251
Copy link

@nickrobinson251 nickrobinson251 commented Oct 28, 2022

@nickrobinson251 nickrobinson251 changed the title Add @ballocs macro for getting number of allocations Add @ballocations macro for getting number of allocations Nov 4, 2022
@nickrobinson251 nickrobinson251 marked this pull request as ready for review November 4, 2022 16:04
@nickrobinson251
Copy link
Author

bump :) is someone available to review this please?

@KristofferC
Copy link
Contributor

I don't really understand the purpose of this macro and the @ballocated one. Why would you run a function potentially thousands of times (until the execution time is within some noise level) to count the number of allocations (which is not noisy). Isn't one for compiling the function and one for counting the allocations enough?

@nickrobinson251
Copy link
Author

nickrobinson251 commented Dec 1, 2022

Isn't one for compiling the function and one for counting the allocations enough?

yeah, i think it probably is. And i intended to use it like @ballocations expression samples=2, and would be happy for this to default to samples=2 (is samples the right one? i guess evals=1 samples=2?)

the purpose is to have a standard, easy-to-use way to test the number of allocations (ignoring the compilation)

since @ballocated exists already, i thought @ballocations made sense (rather than defining such a macro in the tests of multiple packages)

but i think setting default evals/samples could make sense -- should i make that change?

@nickrobinson251
Copy link
Author

i hope to use it the same way @ballocated is already used in e.g. the NamedDims tests: https://github.com/invenia/NamedDims.jl/search?q=ballocated

running these functions many times is no issue (since they're written to be basically no cost), but it's a lot nicer (i.e. less burden on the package developers writing the tests) to use @ballocated rather than having to makes sure things have already compiled and then using @allocated

@gdalle
Copy link
Collaborator

gdalle commented Jun 12, 2023

but i think setting default evals/samples could make sense -- should i make that change?

Sounds like a good idea to me, although it will have to be thoroughly documented for users not to be surprised.

src/execution.jl Outdated Show resolved Hide resolved
and accepts all of the same additional parameters as `@benchmark`.
The returned allocations correspond to the trial with
the *minimum* elapsed time measured during the benchmark.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we set a default value evals=1 samples=2 for this one? And explain it in the docstring?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then again it would be incoherent with the other @bstuff macros so I'm torn

@gdalle gdalle added this to the v2.0 milestone Sep 18, 2023
@gdalle gdalle removed this from the v2.0 milestone Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants