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

fix: Don't break at calculating log of numbers close to zero #103

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

gnufede
Copy link

@gnufede gnufede commented Dec 12, 2023

Issue number of the reported bug or feature request: #102

Describe your changes
Manage the case when calculating log of numbers too close to zero.

Testing performed
None yet.

@gnufede
Copy link
Author

gnufede commented Dec 14, 2023

@pablogsal I know my math teacher wouldn't be very proud of this PR, but I'd love your feedback anyway

@pablogsal
Copy link
Member

Thanks for the PR @gnufede! I’m currently on vacation but I’m sure @godlygeek can take a look soon :)

If the program performs zero byte allocations, pretend that they were
one byte allocations instead. This way they'll be counted in the lowest
histogram bucket, as they should be, but we won't attempt to call
`math.log(0)` and get a math domain error.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
@godlygeek
Copy link
Contributor

Thanks for the bug report. I've gone with a different fix, though. This case would only come up when the program performed an allocation of zero bytes. If we hadn't been causing an exception by calling math.log(0) when that happened, we still would have been handling those zero-byte allocations in an odd way, excluding them from the histogram entirely.

Instead, I've updated this PR to just pretend that zero-byte allocations were one-byte allocations, so that they still get counted in the histogram, and just get slotted into the smallest bin.

@pablogsal Mind giving me a review here, since I've rewritten this PR?

@godlygeek godlygeek requested a review from pablogsal March 6, 2024 22:19
@godlygeek godlygeek merged commit b2d6aa8 into bloomberg:main Mar 6, 2024
10 checks passed
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.

None yet

4 participants