-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 new remote updater for go pserver #2774
Conversation
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.
👍 resolve such a difficult bug! Awesome.
go/pserver/optimizer.go
Outdated
"ConfigSize": len(c), | ||
}).Info("New Optimizer Created with config:") | ||
var cbuffer unsafe.Pointer | ||
cbuffer = C.malloc(C.size_t(len(p.Content))) | ||
C.memcpy(cbuffer, unsafe.Pointer(&p.Content[0]), C.size_t(len(p.Content))) | ||
C.memcpy(cbuffer, unsafe.Pointer(&p.Content[0]), C.size_t(len(p.Content)/C.sizeof_float)) |
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.
it is so strange that we can pass the go test
smoothly.
what's more, C.memcpy
copy four times longer buffer than the real length, it never core dump(system call libc's memcpy in low level), and set all the data to 0 as normal.
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.
Wired, maybe the test case is not exposing this problem?
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.
Oh, no, we malloc four times memory.......
cbuffer = C.malloc(C.size_t(len(p.Content)))
, it absolutly will run smoothly because I waste the rest buffer.
weird
, by the way. : )
… fix_newupdater
LGTM! |
Fix #2761