Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Robert/addr memory leak #8717
Robert/addr memory leak #8717
Changes from 10 commits
b69e19b
280168c
60e6d71
ecc288e
991169a
fbe6b78
2294920
7e577b9
0a3ff3f
3b1f40b
a02c0d4
a890b1e
4d26f57
cbbf8b3
f71691f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To better get a good benchmark in which code won't be optimized away, use a global variable that's an interface e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean a variable outside of any function? We are not using it in other benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well just because it wasn't done correctly before doesn't mean we shouldn't do it correctly, I certainly have submitted many other benchmarks where we set to a global sink and check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked it locally - it doesn't change anything. Are you sure we need to trick the benchmark code?
My only objection is that we make the whole thing less readable and more complex without any observable effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure about code at times getting optimized away and that being the only way to block that. Humbly, for a bit of clarity, I work on the Go project, across almost all the packages and I am bringing this advice from the many years of working on benchmarking related code :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But why to obfuscate more if there is no difference (I verified it)? The
require.NoEmpty
already assures that value must not be ignored.