Remove timestamp allocs #254
Conversation
|
Can't we do better than a sync pool here?
…On Sun, 22 Jan 2017, 03:08 Blake Mizerany, ***@***.***> wrote:
When using gogofaster all of my allocations went away except for stdtime
fields. This fixes that. The bench results are in the commit message.
------------------------------
You can view, comment on, or merge this pull request online at:
#254
Commit Summary
- Add StdTimeUnmarshal benchmark
- Remove all allocations in StdTimeUnmarshal
File Changes
- *M* types/timestamp_gogo.go
<https://github.com/gogo/protobuf/pull/254/files#diff-0> (12)
- *M* types/timestamp_test.go
<https://github.com/gogo/protobuf/pull/254/files#diff-1> (17)
Patch Links:
- https://github.com/gogo/protobuf/pull/254.patch
- https://github.com/gogo/protobuf/pull/254.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#254>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABvsLZd-hWlwMWjVv_6jkc3rhq6ez5Pcks5rUroNgaJpZM4LqPx1>
.
|
|
Basically creating a Timestamp just to Unmarshal it and then convert to another type is a very slow way to simply unmarshal to ints into a time.Time struct. A function can be written to return two ints instead of unmarshaling into an object. You can also turn your benchmark into a test that fails it any allocs occur. |
|
Noted. I'll look into this tomorrow. |
bmizerany
force-pushed
the
removeTimestampAllocs
branch
from
0785920
to
838ef0f
Compare
benchmark old ns/op new ns/op delta BenchmarkStdTimeUnmarshal-4 41.5 18.1 -56.39% benchmark old allocs new allocs delta BenchmarkStdTimeUnmarshal-4 1 0 -100.00% benchmark old bytes new bytes delta BenchmarkStdTimeUnmarshal-4 16 0 -100.00%
bmizerany
force-pushed
the
removeTimestampAllocs
branch
from
838ef0f
to
8e7d033
Compare
|
Okay. Took a quick stab at this tonight and got close to a 10x speed increase after replacing the sync.Pool with a custom function to unmarshal the data. I started to thing making a custom function would/cloud break unmarshaling if anything changed in timestamp.proto since it wouldn't get any updates the generated code would but I concluded that it is unlikely it will change. If anything does break there are enough tests in here to catch it I suppose.
|
| } | ||
|
|
||
| func TestStdTimeUnmarshalAllocs(t *testing.T) { | ||
| data, err := StdTimeMarshal(time.Time{}) |
There was a problem hiding this comment.
how about using NewPopulatedTime, instead of an empty time?
There was a problem hiding this comment.
I'll add another AllocsPer with a time.Now() to prove allocs don't happen for both the zero value and non-zero.
| @@ -135,3 +135,37 @@ func TestTimestampString(t *testing.T) { | |||
| func utcDate(year, month, day int) time.Time { | |||
| return time.Date(year, time.Month(month), day, 0, 0, 0, 0, time.UTC) | |||
| } | |||
|
|
|||
| func BenchmarkStdTimeUnmarshal(b *testing.B) { | |||
| data, err := StdTimeMarshal(time.Time{}) | |||
There was a problem hiding this comment.
NewPopulated and maybe make a few in a slice, if possible?
Is RunParallel necessary?
There was a problem hiding this comment.
What benefit do we get from randomly generated Times? What do you suggest we do with the slice of Times? RunParallel is not necessary and I'll remove.
There was a problem hiding this comment.
I added a non-zero time to the bench and remove RunParallel.
|
Optional:
|
|
I think Duration, Marshaling, etc are better done in another PR to prevent this one from living too long. I'll be getting to those soon when my forcing function kicks in. |
|
I believe I've address all of your concerns about the current patch. It would be nice to get this merged and I'm happy to send over Marshaling and Duration in sperate future PRs. |
|
Ok maybe we can at least take on the Marshal of the timestamp in this commit as well. |
|
hello? |
|
Can I pick this up? I'm in the same situation re: allocs due to StdTime fields. I will implement the marshaling and Duration equivalents if you're still ok with this approach. |
When using gogofaster all of my allocations went away except for stdtime fields. This fixes that. The bench results are in the commit message.