Make State constructor private #650
Conversation
|
✅ Build benchmark 1365 completed (commit bc47c95194 by @dominichamon) |
|
❌ Build benchmark 1420 failed (commit aa4eb0428f by @dominichamon) |
|
❌ Build benchmark 1423 failed (commit ccfba5f3b5 by @dominichamon) |
|
@EricWF Do you think you'll have a chance to review this? |
|
Yeah, I should find time today
On Sep. 10, 2018 8:20 a.m., "Dominic Hamon" <notifications@github.com> wrote:
@EricWF <https://github.com/EricWF> Do you think you'll have a chance to
review this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#650 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABHza71yfGO82XElK_fVVvTqCyvakRH2ks5uZlj8gaJpZM4V6r4S>
.
|
src/benchmark.cc
Outdated
| State st(iters, b->arg, thread_id, b->threads, &timer, manager); | ||
| b->benchmark->Run(st); | ||
| CHECK(st.iterations() >= st.max_iterations) | ||
| std::unique_ptr<State> st = b->Run(iters, thread_id, &timer, manager); |
There was a problem hiding this comment.
I personally somehow don't like this.
src/benchmark_api_internal.cc
Outdated
| std::unique_ptr<State> BenchmarkInstance::Run( | ||
| size_t iters, int thread_id, internal::ThreadTimer* timer, | ||
| internal::ThreadManager* manager) const { | ||
| auto st = std::unique_ptr<State>(new State(iters, arg, thread_id, threads, |
There was a problem hiding this comment.
Well, i should have commented here i guess.
Because it's a memory allocation.
I don't really expect this to matter in the normal usage, but if it can be avoided..
There was a problem hiding this comment.
We could return a copy of the state from the stack. Or we could have a State* out param that the caller can populate with a stack version, but that moves the instantiation to elsewhere and breaks some of the encapsulation of the Run method.
There was a problem hiding this comment.
Just a thought :) I haven't benchmarked this, so i'm likely just paranoid :)
There was a problem hiding this comment.
Well that merge was brutal. It now doesn't have the allocation, but it copies. Given this happens once per instance run (ie, not per iteration) and isn't measured as part of the benchmark, i think this is fine. And simpler.
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
I think CLA is failing because it includes email addresses that aren't explicitly in my profile. Specifically dominichamon@users.noreply.github.com which is the github internal email address. |
|
❌ Build benchmark 1457 failed (commit 4520662985 by @dominichamon) |
|
Double checked CLA and everything is fine. it's all me in the author list. Review please? |
|
github really does not like such merges. |
The State constructor should not be part of the public API. Adding a utility method to BenchmarkInstance allows us to avoid leaking the RunInThread method into the public API.
dmah42
force-pushed
the
private_ctor
branch
from
7623947 to
deae1d9
Compare
|
CLAs look good, thanks! |
|
❌ Build benchmark 1484 failed (commit 6d9e5ac45f by @dominichamon) |
|
appveyor failed due to expectation of 0-6 integers for user counter test time but it's up to 7. |
The State constructor should not be part of the public API. Adding a utility method to BenchmarkInstance allows us to avoid leaking the RunInThread method into the public API.
Stateis part of the public API but should not be instantiable outside the benchmark library.