-
Notifications
You must be signed in to change notification settings - Fork 63
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
Benchmark registration shouldn't be coupled to main.cpp
#39
Comments
You could look at GLib's Constructors for an idea of how to do this somewhat portably, but AFAIK there is just to guaranteed way to do this. I think a better solution would be to just have each group compiled into a separate library which could then be |
@nemequ - I'm not following the problem here. I've used this pattern many times (note I'm talking about C++ here not C). The portable approach is just to use straightforward C++, isn't it? An object with a constructor that registers the benchmarks (push instead of the current pull approach). Perhaps I wasn't clear in the description, but I don't anticipate any issue here really, this is just a task so I remember to do the work. I also like the idea of loading additional benchmarks at runtime from a shared object, but that's really a different, bigger feature (and using C++ in "plugin" interfaces is kind of a mess last time I looked). I added issue #43 to track that. |
Ah right, C++. Sorry, not used to thinking in C++. I don't know about it being a "portable" approach (IIRC there can be some issues if you use them in shared libraries), but yeah it should be safe for what you're talking about here. |
@nemequ - yeah there are definitely gotchas in C++, the big ones being that (a) the order between different compilation units isn't defined, so you can easily blow up if your globals refer to each other during initialization and (b) destruction is similarly messy, especially with shared libraries, and it's easy to accidentally access objects have they have been destroyed (the "easy out" here is often just to ensure the global object destructor never run at all: just "leak" them). What I'm thinking of here is simple though and shouldn't run into those issues. I didn't think much about C specifically, but yeah now that C benchmarks are supported it would be nice to come up with an approach there. One option would to be to have a build step that looks for |
AFAICT the registration code currently needs to be C++ anyways, even if the tests themselves are in C, so that doesn't really matter right now. An API to register stuff in C would be very nice for #43, but if you're At some point it wouldn't be too difficult to add a macro which would support constructors on Windows, most GCC/clang/icc configurations, and suncc (would actually be a good idea for portable-snippets), plus C++, but if #43 happens I don't see why the test built in to uarch-bench couldn't use the same mechanism, then there is no need to maintain multiple paths. |
Right, but you could only run one function or perhaps a fixed list of functions after Just want to check that we're on the same page here. Of course, having different modules already makes the problem a lot better since you have at least one way to decouple things. |
I've been assuming one group per shared library, but if you'd prefer it would be fairly easy to do something like typedef struct UarchCtx_ UarchCtx;
typedef struct UarchGroup_ UarchGroup;
void uarch_ctx_add_group(UarchCtx* ctx, UarchGroup* group);
UarchGroup* uarch_group_new(UarchCtx* ctx, const char* id, const char* description);
void uarch_group_add_bench(UarchGroup* group, const char* id, const char* description, long(* func)(uint64_t), int ops_per_iter); I don't think it's too much to ask the modules to have a registration function which knows about all functions within that module. If you Besides, I think the idea of just dropping a C/C++ file into a directory and having uarch-bench pick it up automatically is more attractive if you're modifying uarch-bench (as you currently have to) than if you're building something in your own source tree, especially when you consider that any build system magic would need to be rewritten, or at least customized, for your project. |
Really, why? That would basically break any code that relies on this standard and widespread feature, which is probably most C++ code. It seems unlikely to me that this is a problem on almost any modern platform. In any case, I really see the two things as orthogonal: there should be a way to load benchmark shared modules at runtime (or perhaps a way to embed uarch bench as a component inside your own application/module) and there should be a way to make the registration of benchmarks from C++ and C within uarch-bench or a module a bit more decoupled from having a master list. I think each feature can more or less live or die on its own. I agree with you that for the modules approach it makes the most sense to call one known function after
Yeah, that's fair. |
You're probably right for this project. AFAIK ELF and PE both support constructors in the runtime linker. My understanding is that there are still some platforms where it isn't supported, but you're not likely to run into them on x86 anyways.
I would think you would want a single code path since it's easier to maintain, but if you're comfortable with two I don't mind. That said, I believe there is still an issue with static libraries you should watch out for. Last time I checked, the linker would skip any static libraries for which you don't actually use any symbols, and a constructor doesn't count as usage (https://ofekshilon.com/2013/04/06/forcing-construction-of-global-objects-in-static-libraries/ comes up from a quick search). So, if you're thinking about creating static libraries for each module to keep code duplication minimal while still embedding the built-in tests that may cause problems.
I don't see any reason to complicated it by using a global constructor, but I guess that would be up to each module. |
Currently every file that defines benchmarks needs to declare a benchmark registration method that is called explicitly in
make_benches
inmain.cpp
which look like:This is unfortunate since it means that otherwise independent lists of benchmarks have to be registered in a common place (increasing merge conflicts for independent code) and it also adds another step to adding a new benchmark file.
We should allow independent registration of benchmarks: ideally simply dropping in a new
.cpp
file that has benchmarks would be enough for it to be picked up. That probably means registration should use some kind of global constructor to register tests from the implementing.cpp
file directly. This the order of such calls aren't defined across translation units, we need to sort on benchmark name or something so that we have a consistent order in the benchmark list regardless of actual registration order.The text was updated successfully, but these errors were encountered: