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

Removes needless overoptimization of strings for DigestInfo #174

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

allada
Copy link
Member

@allada allada commented Jul 12, 2023

We make so many copies of DigestInfo and it's not trivial-copyable because of an optimization that adds around 40 bytes of data to each struct which tries to preserve the string representation of the digest when possible. This optimization seems to be pointless and likely ends up costing more than it saves.

On local testing, I found that the overhead of the LazyInit was slightly more than a single invocation of .str(). Since we usually only call .str() once per DigestInfo, it does not seem like a good idea to assume we would need the hex string more than once per struct.

There is also a significant chance this will give secondary optimizations because without this optimization the struct is trivially-copyable. This enables the optimizer to do even more clever tricks for inlining since it knows it does not need to touch the heap.


This change is Reviewable

@allada allada force-pushed the always-calcualte-hash-on-digest-info branch 3 times, most recently from 1fc0a6f to 434d487 Compare July 12, 2023 16:05
Copy link
Collaborator

@chrisstaite-menlo chrisstaite-menlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 8 of 8 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @allada)


util/common.rs line 116 at r1 (raw file):

        let packed_hash = val.packed_hash;
        Digest {
            hash: hex::encode(packed_hash),

Re-implementing val.str().


util/common.rs line 41 at r3 (raw file):

    /// Possibly the size of the digest in bytes. This should only be trusted
    /// if `truest_size` is true.

nit: typo

@allada allada force-pushed the always-calcualte-hash-on-digest-info branch from 434d487 to 30ced7a Compare July 12, 2023 16:07
Copy link
Member Author

@allada allada 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: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @chrisstaite-menlo)


util/common.rs line 116 at r1 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

Re-implementing val.str().

Yep, in reviewing my own code I noticed this too.

Copy link
Collaborator

@chrisstaite-menlo chrisstaite-menlo 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, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @allada)

We make so many copies of DigestInfo and it's not trivial-copyable
because of an optimization that adds around 40 bytes of data to each
struct which tries to preserve the string representation of the
digest when possible. This optimization seems to be pointless and
likely ends up costing more than it saves.

On local testing, I found that the overhead of the LazyInit was
slightly more than a single invocation of `.str()`. Since we usually
only call `.str()` once per DigestInfo, it does not seem like a good
idea to assume we would need the hex string more than once per struct.

There is also a significant chance this will give secondary
optimizations because without this optimization the struct is
trivially-copyable. This enables the optimizer to do even more clever
tricks for inlining since it knows it does not need to touch the heap.
@allada allada force-pushed the always-calcualte-hash-on-digest-info branch from 30ced7a to 68552e8 Compare July 12, 2023 16:11
Copy link
Member Author

@allada allada 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: 7 of 8 files reviewed, all discussions resolved (waiting on @chrisstaite-menlo)


util/common.rs line 41 at r3 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

nit: typo

Huh, this comment must have been from like the first week of this project, I remember thinking, "It'd be nice to know if we can trust the digest size & sha256" stuff... but then that quickly fell off. This is probably residual.

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot. Much simpler.

Some discussion on this was at #153 (comment)

Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @chrisstaite-menlo)

@allada allada merged commit 4062d1d into master Jul 12, 2023
@allada allada deleted the always-calcualte-hash-on-digest-info branch July 12, 2023 17:08
@aaronmondal aaronmondal mentioned this pull request Jul 12, 2023
11 tasks
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