-
Notifications
You must be signed in to change notification settings - Fork 160
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
daemon/combinator: memoize calculated fingerprints (IDs) #4608
daemon/combinator: memoize calculated fingerprints (IDs) #4608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)
-- commits
line 6 at r1:
s/cominator/combinator
private/path/combinator/combinator.go
line 140 at r1 (raw file):
// take []snet.PathInterface directly. func fingerprint(interfaces []snet.PathInterface, sumBuf []byte) snet.PathFingerprint { h := sha256.New()
suggestion: You could save another couple of allocation by reusing the same hash.Hash
You could bundle the sum buffer and hash.Hash together in one struct for easy passing around
private/path/combinator/combinator.go
line 149 at r1 (raw file):
} } return snet.PathFingerprint(h.Sum(sumBuf[:0]))
Add a comment that this is safe because snet.PathFingerprint is underlying type string.
Also, maybe it is worth to create a compiler check that enforces this. If someone changes it to byte slice -> this will go pufff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @jiceatscion and @oncilla)
Previously, oncilla (Dominik Roos) wrote…
s/cominator/combinator
Done.
private/path/combinator/combinator.go
line 140 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
suggestion: You could save another couple of allocation by reusing the same hash.Hash
You could bundle the sum buffer and hash.Hash together in one struct for easy passing around
Done.
private/path/combinator/combinator.go
line 149 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Add a comment that this is safe because snet.PathFingerprint is underlying type string.
Also, maybe it is worth to create a compiler check that enforces this. If someone changes it to byte slice -> this will go pufff.
Done.
Added the comment, Don't know how to add the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)
private/path/combinator/combinator.go
line 149 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Done.
Added the comment, Don't know how to add the check.
// This check ensures that the underlying type of the PathFingerprint
// is string. This is required to create a copy of the buffer. If it were
// to be moved to a []byte, we need to change the code to create
// a proper copy.
func checkUnderlyingType[S ~string](s S) bool { panic("") }
var _ = checkUnderlyingType(Fingerprint(""))
6aa466e
to
ae5fb3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @oncilla)
private/path/combinator/combinator.go
line 149 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
// This check ensures that the underlying type of the PathFingerprint // is string. This is required to create a copy of the buffer. If it were // to be moved to a []byte, we need to change the code to create // a proper copy. func checkUnderlyingType[S ~string](s S) bool { panic("") } var _ = checkUnderlyingType(Fingerprint(""))
Done.
panic doesn't work ;)
a11877b
to
adead42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 27 files at r3.
Reviewable status: 4 of 31 files reviewed, all discussions resolved (waiting on @jiceatscion)
9b4d892
to
7067f74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, 28 of 28 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)
Do not recalculate IDs frequently. Also re-use buffer for fingerprint calculation. Use slices package for sorting in the cominator.
7067f74
to
8df7fd2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r2, 1 of 27 files at r3, 19 of 28 files at r6, 12 of 12 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)
A profile shows that ID calculation is a big part of the CPU time of the daemon:
Therefore this change memoizes IDs where possible.
Also re-use buffer for fingerprint calculation.
Use slices package for sorting in the combinator.