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
Add logic to access strings used by remote config bpf probe #2983
Add logic to access strings used by remote config bpf probe #2983
Changes from all commits
5c82bf2
5df280b
631a57c
d10cdde
8f9ea2e
bf0c920
e6db1a6
12b145c
a81c6ba
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.
How can we guarantee the Go compiler won't optimize this function away, since it has no 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'm less worried about the function itself being optimized away (it doesn't seem to do it according to the disassembler), and more about the actual lines that access the variables. I observed the behavior this PR is trying to mitigate over a large amount of time across different services (though admittedly only on 1 environment), and in fact, we still have occasional misses (seem to typically be followed by hits).
As a result I changed the access to writing the strings to /dev/null and in fact this removed any amount of misses in my manual testing. It also simplifies the code. So i've pushed this change.
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.
Ok i've further updated this to just use io.Discard
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.
Could
runtime.KeepAlive
be enough here? 🤔The doc is a little bit wierd but it's goal is only to force the compiler to not optimize anything away because it can't know at this stage what is going to happen on the other side, which is the runtime package here
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.
@eliottness I wasn't aware of this function. Looking at the code it just performs a simple
println()
on the passed value which is what we're doing except we won't print the config to our customers service logs.The theory we're accepting is that the string addresses need to be in the core-specific TLB before the hooked function is called. Printing to stdout, or with io.Discard, achieves this.
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.
@grantseltzer I probably badly explained what I was suggesting here: Yes technically there is a call to
println
in there but it's never reach because theif
statement end up being always false. Except the compiler cannot know if the variable will stay false so it cannot optimize the if statement away. In the end, it ends up running just a few instructions where asio.Discard([]byte(str))
does a copy of the string and allocates it on the heap to send it to the io package. In the PR description you where talking about the "critical path for Go DI" so I thought you would welcome a performance boost but now that I think of it it's probably unnecessary. Feel free to take or discard (pun intended) my commentThere 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.
@eliottness Ahh, I see. Let me test to make sure this works at resolving this issue, and would rather use this instead. But yea performance isn't the focus of concern considering it gets called once every 5 seconds.
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.
This won't work, the purpose of the print to io.Discard is that it triggers the actual memory to be paged in. If the println call is skipped, that won't happen.