integrate Go with optimizer library#2560
integrate Go with optimizer library#2560dzhwinter wants to merge 0 commit intoPaddlePaddle:developfrom
Conversation
go/pserver/cclient/cclient.go
Outdated
| content := cArrayToSlice(unsafe.Pointer(param.content), int(param.content_len)) | ||
| pc := pserver.ParameterWithConfig{ | ||
| Param: pserver.Parameter{Name: name, ElementType: et, Content: content}, | ||
| Param: pserver.Parameter{Name: name, ElementType: et, Content: param.content, Length: para.content_len}, |
There was a problem hiding this comment.
I think para is not defined. Does this code compiles?
There was a problem hiding this comment.
fix done. sorry for the typo mistake, can not pass the go link problem yesterday.
| @@ -0,0 +1,13 @@ | |||
| import OptimizerConfig_pb2 as pb | |||
There was a problem hiding this comment.
Is this used to generate a proto encoded file? If we only need the generated proto file, maybe we can just check the generated file under testdata folder.
go/pserver/optimizer.go
Outdated
| o.opt = C.paddle_create_SGD_optimizer(C.double(learning_rate)) | ||
| p := paramWithConfigs.Param | ||
| c := paramWithConfigs.Config | ||
| o.opt = C.paddle_create_optimizer(C.uchar(c), C.int(len(c)), unsafe.Pointer(p.Content), c.int(p.Length), nullPtr, 0) |
There was a problem hiding this comment.
paddle_create_optimizer takes the ownership of the param_buffer argument. So this does not work, because p.Content is a Go pointer, which is tracked by Go garbage collector, it will be garbage collected. We need to make a copy here.
For more pointer information please see: https://golang.org/cmd/cgo/#hdr-Passing_pointers
Another question is what is the benefit of changing the type of p.Content from []byte to *byte:
- Before when
p.Contenthave[]byteas type, inpaddle_send_gradsthecArrayToSlicefunction does not make a copy of the gradient buffer. It just use the buffer as underlying storage of the created slice. So changing the type to*bytedoes not improve performance. - At this line of code, we have to make a copy anyway.
I could be wrong, but I don't see any benefit of changing the type of p.Content from []byte to *byte, it comes with added complexity: need a separate variable p.Length to track length. Plus Go programmers are more familiar with slice ([]byte) than byte pointers (*byte).
There was a problem hiding this comment.
I see. fix the copy done.
Another question is what is the benefit of changing the type of p.Content from []byte to *byte
there is no benefit, c style language prefers pointer type. fix it
go/pserver/service.go
Outdated
| opt *optimizer | ||
| mu sync.Mutex | ||
| // injection from parameter to optimizer | ||
| optMap map[string]*optimizer |
There was a problem hiding this comment.
If we already have optMap and Optimizer owns the parameter memory, we no longer need paramMap.
|
it is accidentally closed after a force update when I resolved confict. l had tried many methods, it can not be reopened, weird : ( |
|
If there is any method that can force update the branch and reopen this PR, Please let me know. |
2 participants
fix2516