-
Notifications
You must be signed in to change notification settings - Fork 151
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
feat: derive Allocative on FixedBytes #531
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.
imo ok since its cfg gated, although ci fails because allocative requires nightly?
Fails on 1.65 because it has higher MSRV |
@Rjected do you want to add it to more types? |
I do, as many as possible |
Yep this will improve profiling on Reth |
@DaniPopes I think this is good for now, will open another PR when I get around to bytes stuff |
@@ -16,8 +16,15 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
rust: ["stable", "beta", "nightly", "1.65"] # MSRV | |||
rust: ["stable", "beta", "nightly"] |
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.
are we updating our MSRV policy?
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.
No, just not testing --all-features
because this feature requires a higher MSRV
Motivation
Recently I was introduced to https://github.com/facebookexperimental/allocative, which seems promising as an easy way to collect granular information about certain data structures' memory usage in reth.
Solution
Introduce the
allocative
feature intoprimitives
, and to start, derive theAllocative
trait on FixedBytes. I can derive this on the rest of the types later.PR Checklist