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

Various refactors #70

Merged
merged 5 commits into from
Mar 20, 2024
Merged

Conversation

bens
Copy link
Collaborator

@bens bens commented Mar 20, 2024

Some of these make more sense when recording allocations as well as durations. These changes should make the memory tracking PR much less bulky.

  • Move Statistics into its own module and parameterise it by type, so it can hold u64s for nanoseconds and usizes for bytes.
  • Compute statistics on demand rather than pre-computing them in Result.
  • Add Reading and Readings types, so it can carry more than just nanoseconds without changing most code.
  • Try a different approach to formatting using the extensibility methods from std.fmt. This inlines a fair bit of code but it seems easier to inspect and modify than being hidden behind a bunch of functions.
  • Add an example for the JSON output.
  • Simplify a few examples to be more focused.

If necessary, I can split these into separate PRs.

Some of these make more sense when recording allocations as well as durations.
@bens bens requested a review from hendriknielaender March 20, 2024 09:13
@hendriknielaender
Copy link
Owner

Really like this PR 👍. Much more cleaner that we have now Statistics in its own module. Will check it later today - but at first sight it looks good.

Forgot this earlier, should have been done in the incremental PR.
@hendriknielaender hendriknielaender merged commit ac1b084 into hendriknielaender:main Mar 20, 2024
5 checks passed
@bens bens deleted the refactor branch March 21, 2024 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants