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

Issue 102680045: code review 102680045: encoding/gob: remove unsafe, use reflection. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by r
Modified:
9 years, 9 months ago
Reviewers:
rsc
CC:
rsc, golang-codereviews, khr
Visibility:
Public.

Description

encoding/gob: remove unsafe, use reflection. This removes a major unsafe thorn in our side, a perennial obstacle to clean garbage collection. Not coincidentally: In cleaning this up, several bugs were found, including code that reached inside by-value interfaces to create pointers for pointer-receiver methods. Unsafe code is just as advertised. Performance of course suffers, but not too badly. The Pipe number is more indicative, since it's doing I/O that simulates a network connection. Plus these are end-to-end, so each end suffers only half of this pain. The edit is pretty much a line-by-line conversion, with a few simplifications and a couple of new tests. There may be more performance to gain. BenchmarkEndToEndByteBuffer 2493 3033 +21.66% BenchmarkEndToEndPipe 4953 5597 +13.00% Fixes issue 5159.

Patch Set 1 #

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

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

Total comments: 24

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+593 lines, -725 lines) Patch
M src/pkg/encoding/gob/codec_test.go View 17 chunks +137 lines, -174 lines 0 comments Download
M src/pkg/encoding/gob/debug.go View 2 chunks +3 lines, -3 lines 0 comments Download
M src/pkg/encoding/gob/decode.go View 29 chunks +259 lines, -329 lines 0 comments Download
M src/pkg/encoding/gob/encode.go View 16 chunks +113 lines, -211 lines 0 comments Download
M src/pkg/encoding/gob/encoder_test.go View 2 chunks +74 lines, -2 lines 0 comments Download
M src/pkg/encoding/gob/gobencdec_test.go View 3 chunks +4 lines, -3 lines 0 comments Download
M src/pkg/encoding/gob/timing_test.go View 1 2 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 5
r
Hello rsc (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
9 years, 9 months ago (2014-06-27 17:59:24 UTC) #1
bradfitz
I'm curious how the numbers look with Keith's https://codereview.appspot.com/52670045/ patched in to shrink reflect.Value back ...
9 years, 9 months ago (2014-06-27 18:24:03 UTC) #2
rsc
LGTM Fine to do any/all of this in a followup. https://codereview.appspot.com/102680045/diff/40001/src/pkg/encoding/gob/decode.go File src/pkg/encoding/gob/decode.go (left): https://codereview.appspot.com/102680045/diff/40001/src/pkg/encoding/gob/decode.go#oldcode7 ...
9 years, 9 months ago (2014-06-27 23:54:49 UTC) #3
r
did the cosmetic ones. will do the more semantic ones in a follow-up change. https://codereview.appspot.com/102680045/diff/40001/src/pkg/encoding/gob/decode.go ...
9 years, 9 months ago (2014-06-30 17:08:24 UTC) #4
r
9 years, 9 months ago (2014-06-30 18:06:53 UTC) #5
*** Submitted as https://code.google.com/p/go/source/detail?r=4008fd164bca ***

encoding/gob: remove unsafe, use reflection.
This removes a major unsafe thorn in our side, a perennial obstacle
to clean garbage collection.
Not coincidentally: In cleaning this up, several bugs were found,
including code that reached inside by-value interfaces to create
pointers for pointer-receiver methods. Unsafe code is just as
advertised.

Performance of course suffers, but not too badly. The Pipe number
is more indicative, since it's doing I/O that simulates a network
connection. Plus these are end-to-end, so each end suffers
only half of this pain.

The edit is pretty much a line-by-line conversion, with a few
simplifications and a couple of new tests. There may be more
performance to gain.

BenchmarkEndToEndByteBuffer     2493          3033          +21.66%
BenchmarkEndToEndPipe           4953          5597          +13.00%

Fixes issue 5159.

LGTM=rsc
R=rsc
CC=golang-codereviews, khr
https://codereview.appspot.com/102680045
Sign in to reply to this message.

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