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

mem: skip defer for performance #48

Merged
merged 1 commit into from
Sep 4, 2016

Conversation

abraithwaite
Copy link

We're looking to move to go-capnproto2 from the original and we've been comparing the performance of the two using alecthomas' go serialization benchmark suite.

This is a minor patch which improves the speed moderately. We're still looking at options for increasing the speed. Most of them probably involve changing the go structs to pointers, if that's amenable to you.

Here's the performance comparison with and without this changeset:

https://gist.github.com/abraithwaite/a8876d351edf16bc052cd6b5acdaa330

I'm not sure why the svgs don't render nicely on gist though :-(

@zombiezen
Copy link
Contributor

How I wish that defer was faster. This seems fine.

I remember Jason using that benchmark suite to compare perf numbers before. You may also be interested in running go test -bench='BenchmarkUnmarshal_Reuse', which makes even less allocations with an increase in potential complexity. I haven't gotten around to upstreaming those.

I'm curious: why will converting the structs to pointers make perf improvements? I recently made a largeish API change to remove allocations in the hot paths because it was adversely impacting perf.

@abraithwaite
Copy link
Author

My mistake. My expectation is that by not allocating the metadata structs it'll speed it up a bit.

@@ -168,21 +168,27 @@ func (m *Message) NumSegments() int64 {
// Segment returns the segment with the given ID.
func (m *Message) Segment(id SegmentID) (*Segment, error) {
m.mu.Lock()
defer m.mu.Unlock()
var seg *Segment
if isInt32Bit() && id > maxInt32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this case doesn't require the mutex

Copy link
Author

Choose a reason for hiding this comment

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

You mean we can move m.mu.Lock() below this block correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup.

@abraithwaite
Copy link
Author

abraithwaite commented Aug 29, 2016

Coming back to this:

why will converting the structs to pointers make perf improvements?

You may also be interested in running go test -bench='BenchmarkUnmarshal_Reuse'

That's exactly what I was looking for. Only just got around to reading that code. I also made a patch for the the serialization benchmarks repo. Here are the results: https://gist.github.com/abraithwaite/905555e81b35eca8d30987690dfc1220

Very nice! 👏

Edit: here's the patch, if you want to upstream. Otherwise I'd be happy to, but it's your work :-)

https://gist.github.com/abraithwaite/93d6f0b6ed505a78d6686bff1850eb4a

@zombiezen zombiezen merged commit f8b915c into capnproto:master Sep 4, 2016
zombiezen added a commit that referenced this pull request Sep 4, 2016
mem.go: skip defer for performance
@zombiezen
Copy link
Contributor

Merged with some tweaks to critical sections.

You should go ahead and upstream the benchmark changes.

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