Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1328)

Issue 6750053: code review 6750053: encoding/binary: when reading, skip blank fields (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 5 months ago by gri
Modified:
11 years, 5 months ago
Reviewers:
CC:
r, rsc, golang-dev
Visibility:
Public.

Description

encoding/binary: skip blank fields when (en/de)coding structs - minor unrelated cleanups - performance impact in the noise benchmark old ns/op new ns/op delta BenchmarkReadSlice1000Int32s 83462 83346 -0.14% BenchmarkReadStruct 4141 4247 +2.56% BenchmarkReadInts 1588 1586 -0.13% BenchmarkWriteInts 1550 1489 -3.94% BenchmarkPutUvarint32 39 39 +1.02% BenchmarkPutUvarint64 142 144 +1.41% benchmark old MB/s new MB/s speedup BenchmarkReadSlice1000Int32s 47.93 47.99 1.00x BenchmarkReadStruct 16.90 16.48 0.98x BenchmarkReadInts 18.89 18.91 1.00x BenchmarkWriteInts 19.35 20.15 1.04x BenchmarkPutUvarint32 101.90 100.82 0.99x BenchmarkPutUvarint64 56.11 55.45 0.99x Fixes issue 4185.

Patch Set 1 #

Patch Set 2 : diff -r b13d68941de6 https://code.google.com/p/go #

Patch Set 3 : diff -r 8d919bfe75d3 https://code.google.com/p/go #

Patch Set 4 : diff -r 8d919bfe75d3 https://code.google.com/p/go #

Patch Set 5 : diff -r 8d919bfe75d3 https://code.google.com/p/go #

Total comments: 2

Patch Set 6 : diff -r 2da23e50774a https://code.google.com/p/go #

Patch Set 7 : diff -r d9a624c22a62 https://code.google.com/p/go #

Patch Set 8 : diff -r d9a624c22a62 https://code.google.com/p/go #

Patch Set 9 : diff -r 956500178964 https://code.google.com/p/go #

Patch Set 10 : diff -r 4c898bf605a5 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -23 lines) Patch
M src/pkg/encoding/binary/binary.go View 1 2 3 4 5 6 7 8 9 8 chunks +41 lines, -8 lines 0 comments Download
M src/pkg/encoding/binary/binary_test.go View 1 2 3 4 5 2 chunks +66 lines, -15 lines 0 comments Download

Messages

Total messages: 7
gri
Hello r@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
11 years, 5 months ago (2012-10-22 22:55:24 UTC) #1
gri
friendly ping - gri On Mon, Oct 22, 2012 at 3:55 PM, <gri@golang.org> wrote: > ...
11 years, 5 months ago (2012-10-29 17:25:08 UTC) #2
rsc
This looks fine but it means that Read and Write are asymmetric. Write should write ...
11 years, 5 months ago (2012-11-01 16:31:20 UTC) #3
gri
Made write symmetric and added extra test. Note that removing the use of CanSet causes ...
11 years, 5 months ago (2012-11-01 18:15:23 UTC) #4
rsc
You were right about CanSet: I misread the || as &&. It's fine to put ...
11 years, 5 months ago (2012-11-01 18:27:46 UTC) #5
rsc
LGTM
11 years, 5 months ago (2012-11-01 18:28:35 UTC) #6
gri
11 years, 5 months ago (2012-11-01 19:39:22 UTC) #7
*** Submitted as http://code.google.com/p/go/source/detail?r=1c9411647150 ***

encoding/binary: skip blank fields when (en/de)coding structs

- minor unrelated cleanups
- performance impact in the noise

benchmark                       old ns/op    new ns/op    delta
BenchmarkReadSlice1000Int32s        83462        83346   -0.14%
BenchmarkReadStruct                  4141         4247   +2.56%
BenchmarkReadInts                    1588         1586   -0.13%
BenchmarkWriteInts                   1550         1489   -3.94%
BenchmarkPutUvarint32                  39           39   +1.02%
BenchmarkPutUvarint64                 142          144   +1.41%

benchmark                        old MB/s     new MB/s  speedup
BenchmarkReadSlice1000Int32s        47.93        47.99    1.00x
BenchmarkReadStruct                 16.90        16.48    0.98x
BenchmarkReadInts                   18.89        18.91    1.00x
BenchmarkWriteInts                  19.35        20.15    1.04x
BenchmarkPutUvarint32              101.90       100.82    0.99x
BenchmarkPutUvarint64               56.11        55.45    0.99x

Fixes issue 4185.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/6750053
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b