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

all: avoid copying arrays in loops #17265

Merged
merged 1 commit into from Aug 7, 2018

Conversation

Projects
None yet
3 participants
@cristaloleg
Copy link
Contributor

cristaloleg commented Jul 29, 2018

Avoid copying arrays of heavy structs (100 bytes and more) in loops. Found by the go-critic linter.
Linter's log:

check-package: $GOPATH/src/github.com/ethereum/go-ethereum/swarm/api/manifest.go:162:2: rangeExprCopy: copy of trie.entries (2056 bytes) can be avoided with &trie.entries
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/swarm/api/manifest.go:311:2: rangeExprCopy: copy of mt.entries (2056 bytes) can be avoided with &mt.entries
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/swarm/api/manifest.go:365:2: rangeExprCopy: copy of mt.entries (2056 bytes) can be avoided with &mt.entries

check-package: $GOPATH/src/github.com/ethereum/go-ethereum/trie/node.go:58:2: rangeExprCopy: copy of n.Children (272 bytes) can be avoided with &n.Children
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/trie/node.go:101:2: rangeExprCopy: copy of n.Children (272 bytes) can be avoided with &n.Children
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/trie/trie.go:359:3: rangeExprCopy: copy of n.Children (272 bytes) can be avoided with &n.Children

check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discover/table.go:163:2: rangeExprCopy: copy of tab.buckets (136 bytes) can be avoided with &tab.buckets
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discover/table.go:511:2: rangeExprCopy: copy of tab.buckets (136 bytes) can be avoided with &tab.buckets
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discover/table.go:527:2: rangeExprCopy: copy of tab.buckets (136 bytes) can be avoided with &tab.buckets
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discover/table.go:536:2: rangeExprCopy: copy of tab.buckets (136 bytes) can be avoided with &tab.buckets

check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discover/table_test.go:598:2: rangeExprCopy: copy of tn.dists (6168 bytes) can be avoided with &tn.dists

check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discv5/table.go:84:2: rangeExprCopy: copy of tab.buckets (2056 bytes) can be avoided with &tab.buckets
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discv5/table.go:96:2: rangeExprCopy: copy of tab.buckets (2056 bytes) can be avoided with &tab.buckets
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discv5/table.go:124:2: rangeExprCopy: copy of tab.buckets (2056 bytes) can be avoided with &tab.buckets
check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discv5/table.go:178:2: rangeExprCopy: copy of tab.buckets (2056 bytes) can be avoided with &tab.buckets

check-package: $GOPATH/src/github.com/ethereum/go-ethereum/p2p/discv5/net_test.go:358:2: rangeExprCopy: copy of tn.dists (6168 bytes) can be avoided with &tn.dists

@cristaloleg cristaloleg requested review from fjl and zsfelfoldi as code owners Jul 29, 2018

@quasilyte

This comment has been minimized.

Copy link
Contributor

quasilyte commented Jul 29, 2018

It's not slice copy, but rather array copy, if we're talking about rangeExprCopy. 😉

@cristaloleg cristaloleg changed the title all: avoid copying slices in loops all: avoid copying arrays in loops Jul 29, 2018

@cristaloleg

This comment has been minimized.

Copy link
Contributor Author

cristaloleg commented Jul 29, 2018

@quasilyte thanks, I've fixed title & description.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Aug 3, 2018

@cristaloleg Are you sure this isn't automatically optimized by the compiler? Would seem really weird if it actually did a full on copy of the arrays.

@quasilyte

This comment has been minimized.

Copy link
Contributor

quasilyte commented Aug 3, 2018

@karalabe I'm sure and tried to fix it in the compiler. Possible to be optimized, but you need to know that loop is never executed from different threads, otherwise you get different results if array is modified. By go specs, it must be a full copy there.

@cristaloleg

This comment has been minimized.

Copy link
Contributor Author

cristaloleg commented Aug 3, 2018

There is an issue for that: golang/go#15812
(Sadly it's postponed from 1.8 till..)

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Aug 7, 2018

Fair enough!

@karalabe
Copy link
Member

karalabe left a comment

LGTM

@karalabe karalabe added this to the 1.8.14 milestone Aug 7, 2018

@karalabe karalabe merged commit cf05ef9 into ethereum:master Aug 7, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cristaloleg cristaloleg deleted the cristaloleg:avoid-slice-copying-in-loops branch Aug 7, 2018

@cristaloleg

This comment has been minimized.

Copy link
Contributor Author

cristaloleg commented Aug 7, 2018

Thanks to everyone!

justelad added a commit to ethersphere/go-ethereum that referenced this pull request Aug 16, 2018

firmianavan added a commit to firmianavan/go-ethereum that referenced this pull request Aug 28, 2018

@HAOYUatHZ HAOYUatHZ referenced this pull request Sep 1, 2018

Merged

Fix #1310

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.