-
Notifications
You must be signed in to change notification settings - Fork 10
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
[WIP] malloc optimizations #84
base: main
Are you sure you want to change the base?
Conversation
Compiler optimizations don't seem to kick in - the Component struct is copied with the value methods, and the Typ struct is also allocated on the heap.
PIT entries once removed cannot be reused, even for removing. Since remove is only called from the priority queue expiration function, this should not be a problem.
At this point most allocations come from the TLV decoder.
|
"value.%[1]s, err = context.%[1]s_context.Parse(reader.Delegate(int(l)), ignoreCritical)", | ||
f.name, | ||
), nil | ||
code := fmt.Sprintf("drdr := reader.Delegate(int(l))\n") |
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.
If you create a new variable, I think it is better to surround it with {...}
to avoid potential conflict, like:
ndnd/std/encoding/codegen/fields_natural.go
Lines 57 to 61 in e5c2033
g.printlnf("{") | |
g.printlnf("tempVal := uint64(0)") | |
g.printlne(GenNaturalNumberDecode("tempVal")) | |
g.printlnf("value.%s = &tempVal", f.name) | |
g.printlnf("}") |
Based on the benchmark https://github.com/zjkmxy/ndn-encoding-test |
I'll set up a benchmark and report 👍🏻 I do expect a slowdown in certain cases, e.g. if the benchmark is single-threaded and/or there is no GC pressure. Any benefits would likely only show up when GC itself is contending for CPU time with decoding. |
@zjkmxy So I ran the benchmark and it does behave as I expected. The issue is this isn't easy to test. Since the "test" packets in the benchmark are on the live heap, this inflates the target heap size (see), so GC practically never happens in the benchmark.
GOGC=1 GOMAXPROCS=1 ./ndn-encoding-test.exe With this, decoding time goes from ~4.50s to ~3.95s (average of 5 runs). This said, the gains do seem somewhat academic here - unlike my assumption, the malloc itself is extremely fast. I'm OK to abandon BTW - this benchmark is great! I'll explore integrating it as an automated test here if/when time permits. PS: yes I'm testing on Windows :p |
I don't know to be honest. The benchmark is for encoding & decoding only and does not consider the effect of forwarder pipeline. If possible, I think we still need a forwarding stress test.
All data I put into that repo are on Bruins. It can still reproduce the old data if you roll back to an old commit to use zjkmxy/go-ndn. |
|
||
var pitCsTreeNodePool = sync.Pool{New: newPitCsTreeNode} | ||
|
||
func newPitCsTreeNode() interface{} { |
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.
nit: I think any
is preferred over interface{}
for new code.
Compiler optimizations don't seem to kick in
Allocations observed 2GB transfers later -