-
Notifications
You must be signed in to change notification settings - Fork 58
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
Store refactor #1010
Store refactor #1010
Conversation
!gpu-benchmark |
1 similar comment
!gpu-benchmark |
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.
It would be nice to have the output of cargo criterion --bench end2end
on this branch relative to the numbers from main
so we have more data about the consequences of these changes
!gpu-benchmark |
d128fdc
to
cc602b2
Compare
Benchmark for 031d3a6Click to view benchmark
|
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.
Sorry, more comments on docstrings and I think it will be ready to go
3e4479a
to
70ee021
Compare
Wonderful. Thank you very much! The last thing: running As a suggestion, the performance diff can be part of the PR description so it's easier to track such perf changes (or absence of changes). |
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.
This PR adds a raw pointer type, which is the payload for the pointer type (that carries a tag as well). We can understand a raw pointer as a delayed hash, which is ultimately a field element once you hydrate.
The store is also changed so that it uses raw pointers instead of pointers. This is because we can view tags as field elements, which is a particular kind of raw pointer. This gives more flexibility to the store since it can now represent hashes where the first element is not necessarily a tag, but a general field element (or raw pointer). This will be used, for example, for the environment optimization, which will replace environment conses with a special operation which is a hash of 3 raw values and 1 tag. [...] The [...] store will also give LEM the ability of expressing pure field elements, with delayed hashing, without needing to create dummy tag variables.
The above is excellent documentation, because it pulls in one place several pieces (that are documented in several places) and ties them up at a high level. I'd consider leaving an edited version of this in a module comment (e..g lem::store?)
- This is trying to use const generics (which are currently sorely limited, see e.g.) to link our hash arity to the hashing API of neptune, which morally is using
generic_array
to represent that same arity. Performing that link may not be necessary today, but FWIW it is a solved problem (as much as is possible in today's compiler) in https://github.com/RustCrypto/utils/tree/master/hybrid-array
&self, | ||
raw_ptrs: &[RawPtr; N], | ||
) -> Option<[Ptr; P]> { | ||
assert_eq!(P * 2, N); |
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.
Yeah, generic_const_exprs
is sorely missing here.
Thankfully, you're using this for array operations, and we already have generic-array in our dependencies. We're therefore in the special case where we can use this obscure but beautiful crate, which I believe is the only way to use const generic array operations with a path to const generic migration when the Rust compiler supports it:
https://docs.rs/hybrid-array/latest/hybrid_array/
https://github.com/RustCrypto/utils/tree/master/hybrid-array
cc5d9b5
to
bfc78a0
Compare
aad6df6
to
c25d5fb
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.
These are the sensible numbers from my machine:
end2end_benchmark/end2end_go_base_nova/_10_0
time: [626.69 ms 627.41 ms 628.10 ms]
change: [-0.5097% -0.3768% -0.2495%] (p = 0.00 < 0.05)
Change within noise threshold.
store_benchmark/store_go_base_pallas/_10_16
time: [157.07 µs 157.15 µs 157.22 µs]
change: [+3.0513% +3.0999% +3.1489%] (p = 0.00 < 0.05)
Performance has regressed.
store_benchmark/store_go_base_pallas/_10_160
time: [155.36 µs 155.44 µs 155.51 µs]
change: [-0.1862% -0.0101% +0.1333%] (p = 0.91 > 0.05)
No change in performance detected.
hydration_benchmark/hydration_go_base_pallas/_10_16
time: [884.80 ns 884.86 ns 884.91 ns]
change: [+0.3267% +0.3518% +0.3757%] (p = 0.00 < 0.05)
Change within noise threshold.
hydration_benchmark/hydration_go_base_pallas/_10_160
time: [884.53 ns 884.62 ns 884.72 ns]
change: [+0.3261% +0.3548% +0.3833%] (p = 0.00 < 0.05)
Change within noise threshold.
eval_benchmark/eval_go_base_pallas/_10_16
time: [6.7053 ms 6.7144 ms 6.7287 ms]
change: [+1.0284% +1.1951% +1.3932%] (p = 0.00 < 0.05)
Performance has regressed.
eval_benchmark/eval_go_base_pallas/_10_160
time: [66.937 ms 67.014 ms 67.109 ms]
change: [-1.1014% -0.9183% -0.7256%] (p = 0.00 < 0.05)
Change within noise threshold.
They look fine to me. Great work!
pos
and index
(from the last commit) are pretty much screaming for a roundtrip test. After that I will stamp this PR
4b40af9
to
337ef0a
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.
🏬 🚚 🏪
match self { | ||
RawPtr::Atom(x) => Some(*x), | ||
_ => None, | ||
} |
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.
With match_opt!
you can turn this into
match_opt!(self, RawPtr::Atom(x) => *x)
} | ||
} | ||
|
||
pub fn pos(i: usize) -> Option<Self> { |
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.
We already have a tag <-> usize for each of the basic enums you're papering over. Have you thought about making this conversion defined in terms of those individual conversions plus a trio of Expr_offset == 0
, Cont_offset = 12
Op1_offset
, Op2_offset
, etc ..?
Another approach would be a proc_macro that walks the definition of the Tag
enum.
My problem is that with this code, the maintenance is going to be a pain: any mis-ordering of fields will be a bug.
def5463
to
250b2cc
Compare
This PR adds a raw pointer type, which is the payload for the pointer type (that carries a tag as well). We can understand a raw pointer as a delayed hash, which is ultimately a field element once you hydrate.
The store is also changed so that it uses raw pointers instead of pointers. This is because we can view tags as field elements, which is a particular kind of raw pointer. This gives more flexibility to the store since it can now represent hashes where the first element is not necessarily a tag, but a general field element (or raw pointer). This will be used, for example, for the environment optimization, which will replace environment conses with a special operation which is a hash of 3 raw values and 1 tag. I've previously done this on this branch but it was a hacky solution on the old store. The new store will also give LEM the ability of expressing pure field elements, with delayed hashing, without needing to create dummy tag variables.
I tried making the API as similar as possible, but there were some changes here and there. I've also tried to make it more generic, to make it easier to add new hashes. There are some functions that use const generics which could use a little polishing; I've written some notes on the comments