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

daemon/combinator: memoize calculated fingerprints (IDs) #4608

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Sep 2, 2024

A profile shows that ID calculation is a big part of the CPU time of the daemon:
image

Therefore this change memoizes IDs where possible.
Also re-use buffer for fingerprint calculation.
Use slices package for sorting in the combinator.

@jiceatscion
Copy link
Contributor

This change is Reviewable

Copy link
Contributor

@oncilla oncilla left a 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.

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a 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)


-- commits line 6 at r1:

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.

Copy link
Contributor

@jiceatscion jiceatscion left a 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)

Copy link
Contributor

@oncilla oncilla left a 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)

Copy link
Contributor

@oncilla oncilla left a 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(""))

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a 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 ;)

Copy link
Contributor

@oncilla oncilla left a 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)

@lukedirtwalker lukedirtwalker force-pushed the combinator-fixes branch 3 times, most recently from 9b4d892 to 7067f74 Compare September 3, 2024 09:00
Copy link
Contributor

@oncilla oncilla left a 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: :shipit: 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.
Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)

@lukedirtwalker lukedirtwalker merged commit 9ceb22a into scionproto:master Sep 24, 2024
5 checks passed
@lukedirtwalker lukedirtwalker deleted the combinator-fixes branch September 24, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants