API redesign #61
API redesign #61
Conversation
bens
force-pushed
the
api-redesign
branch
2 times, most recently
from
6abb394 to
a4e61f9
Compare
|
AFAICT I've implemented all the features of the current library, and added a couple of things which could be moved into separate PRs to simplify review. Those are
I made a I can take out those three changes for now but I might hold off on making PRs for them to avoid needless conflicts if some version of this PR gets merged. I'd love a thorough review, there's obviously a lot changed but I wasn't able to make small changes to get to the desired API. Reviewing the diffs probably won't be very helpful, best just to check out the branch and play around. |
|
Ok, I've moved those three features to separate branches based on this api-redesign branch, so they can wait for later I think. The diff is a little less onerous now... |
There was a problem hiding this comment.
Hey @bens, first of all really cool PR. Really appreciate that you tackled this 💪. This API looks already a lot cleaner.
For the upcoming PR, especially for the json output, i opened an issue please have a look if you agree with the feature request. #63
Maybe we can also go first with your already prepared PR 👍 and then tackle the issue.
There was a problem hiding this comment.
not much more to say except I like it ^^ I'm pretty much inactive at the moment when it comes to Zig 0.11, only using the 0.12-dev version for my little projects. Once 0.12 is released as "stable", I'll re-work some zBench benchmarks and will probably have more to contribute here ;-)
| @@ -0,0 +1,5 @@ | |||
| const std = @import("std"); | |||
|
|
|||
| pub const Error = std.mem.Allocator.Error; | |||
There was a problem hiding this comment.
would call this "MemErr" or something along that line, to differentiate between other, "normal" errors
There was a problem hiding this comment.
The idea was it's any error that can be thrown from a runner, but currently that's only allocation errors.
util/platform.zig
Outdated
| @@ -30,6 +56,7 @@ pub fn linux(allocator: std.mem.Allocator) !OsInfo { | |||
| const memory = try lnx.getTotalMemory(); | |||
|
|
|||
| return OsInfo{ | |||
| .allocator = allocator, | |||
There was a problem hiding this comment.
since we're "fixing" the allocator now in OsInfo, what about making getSystemInfo a method of this struct? The platform-specific implementations could also be non-public.
There was a problem hiding this comment.
I had some other ideas around the OsInfo thing but moved them to another branch, this was a small change to make the structure usable as not just an internal thing. See bens@59f8d7a for the other ideas, basically I wanted to avoid needing an allocator, only use stack while fetching the info and then cache it in static memory. Then it never needs to be deinited.
Currently the whole API is in the zbench module, moving getSystemInfo to zbench.util.platform.OsInfo is a little more obscure, otherwise I don't mind.
|
One last request, can you update the README. Since the config changes a bit 👍 |
- Make the Benchmark type manage any number of benchmark functions and run them all with a single call to `run()`. - Provide all recorded timing information in `Results`, so any desired analysis and output formatting is available to the library user. - Made the running logic more testable by converting it to an iterator type interface - it doesn't actually run the code under test itself now. - Avoid needing an allocator when getting system info, it's only fetched once and then cached in static memory now.
Taking a stab at #52, I had the exact same API in mind before I saw that issue. Currently it's just a basic draft but it works for the most basic case.