Skip to content
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

Manually marshal packets for more speed #18

Merged
merged 2 commits into from
Sep 28, 2014
Merged

Conversation

rakoo
Copy link
Contributor

@rakoo rakoo commented Sep 25, 2014

We change the sendPacket function to accept a encoding.BinaryMashaler and implement the related MarshalBinary function on each packet type. This gives us great speed and allocation wins (see #16) at the expense of less readability, albeit one-time only.

@davecheney
Copy link
Member

Looks good except for one problem, types_like_this_are_not_considered_idiomatic, please use sftpSomethingSomethingSomething.

@rakoo
Copy link
Contributor Author

rakoo commented Sep 26, 2014

Reworked initial commit for CamelCase over underscore_case and added a go fmt run on attrs.go

func sendPacket(w io.Writer, m encoding.BinaryMarshaler) error {
bb, err := m.MarshalBinary()
if err != nil {
return errors.New(fmt.Sprintf("marshal2(%#v): binary marshaller failed", err))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return fmt.Errorf(...)

@davecheney
Copy link
Member

LGTM. Just one little nit then I can merge this.

Thank you for this, it will help #15 a lot.

@rakoo
Copy link
Contributor Author

rakoo commented Sep 27, 2014

Wow, didn't know about fmt.Errorf, thanks for the tip !

davecheney added a commit that referenced this pull request Sep 28, 2014
Manually marshal packets for more speed
@davecheney davecheney merged commit 09a8c3d into pkg:master Sep 28, 2014
@davecheney
Copy link
Member

Lovely. Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants