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

chore: factor out StoreCore #1240

Merged
merged 1 commit into from
Apr 17, 2024
Merged

chore: factor out StoreCore #1240

merged 1 commit into from
Apr 17, 2024

Conversation

arthurpaulino
Copy link
Contributor

@arthurpaulino arthurpaulino commented Apr 16, 2024

StoreCore is a data structure that can encode data as DAGs of tagged pointers, which can be content-addressed with a custom hasher. The tag type as well as the digest type are generic.

To accomplish this, all pointer types are now unified in a single type hierarchy.

@arthurpaulino arthurpaulino force-pushed the ap/store-core branch 3 times, most recently from 770422c to 6bf83ac Compare April 16, 2024 22:47
`StoreCore` is a data structure that can encode data as DAGs of
tagged pointers, which can be content-addressed with a custom
hasher. The tag type as well as the digest type are generic.

To accomplish this, all pointer types are now unified in a single
type hierarchy.
@arthurpaulino arthurpaulino linked an issue Apr 17, 2024 that may be closed by this pull request
@arthurpaulino arthurpaulino changed the title Store core chore: factor out StoreCore Apr 17, 2024
@arthurpaulino
Copy link
Contributor Author

!benchmark --features cuda

@arthurpaulino arthurpaulino marked this pull request as ready for review April 17, 2024 14:27
@arthurpaulino arthurpaulino requested review from a team as code owners April 17, 2024 14:27
Copy link
Contributor

Benchmark for bab22ba

Click to view benchmark
Test Base PR %
LEM Fibonacci Prove - rc = 100/fib/num-100-aed8503cc93afff1d81d7ea1ceb0eeab0454a122-2024-04-17 1497.8±11.46ms N/A N/A
LEM Fibonacci Prove - rc = 100/fib/num-100-bab22ba2eb600c8f83c9d30b439084537a0f3b53-2024-04-15 1477.1±6.75ms N/A N/A
LEM Fibonacci Prove - rc = 100/fib/num-200-aed8503cc93afff1d81d7ea1ceb0eeab0454a122-2024-04-17 2.8±0.01s N/A N/A
LEM Fibonacci Prove - rc = 100/fib/num-200-bab22ba2eb600c8f83c9d30b439084537a0f3b53-2024-04-15 2.8±0.01s N/A N/A

Copy link
Contributor

!gpu-benchmark action succeeded! 🚀

https://github.com/lurk-lab/lurk-rs/actions/runs/8723674973

@arthurpaulino
Copy link
Contributor Author

Hmm, that's bad. Hold on

@arthurpaulino
Copy link
Contributor Author

Nvm, it's okay.

This is from https://github.com/lurk-lab/lurk-rs/actions/runs/8723674973/job/23932441551:

LEM Fibonacci Prove - rc = 100/fib/num-100 (base): 1477.1±6.75ms
LEM Fibonacci Prove - rc = 100/fib/num-100 (pr): 1497.8±11.46ms

LEM Fibonacci Prove - rc = 100/fib/num-200 (base): 2.8±0.01s
LEM Fibonacci Prove - rc = 100/fib/num-200 (p2): 2.8±0.01s

Comment on lines +55 to +67
fn default() -> Self {
Self {
atom: Default::default(),
tuple2: Default::default(),
tuple3: Default::default(),
tuple4: Default::default(),
hasher: Default::default(),
comms: Default::default(),
dehydrated: Default::default(),
z_cache: Default::default(),
inverse_z_cache: Default::default(),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you derive this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so because it requires further implementations for T and D

Comment on lines +262 to +268
fn hydrate_z_cache_with_ptr_vals(&self, ptrs: &[&IVal]) {
ptrs.chunks(256).for_each(|chunk| {
chunk.par_iter().for_each(|ptr| {
self.hash_ptr_val_unsafe(ptr);
});
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to have some explicit topological sorting mechanism, like what happens in the current implementation of provenance hashing: https://github.com/lurk-lab/lurk-rs/blob/bab22ba2eb600c8f83c9d30b439084537a0f3b53/src/coroutine/memoset/mod.rs#L727-L746

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to think about this, though: we want to avoid unnecessary hashing, and when we are no longer using all hashes for internal structure, that means there may well be pointers in the store that never need to be hashed. One strategy for proofs may involve ensuring that the store contains no superfluous hashes — so all those it does contain can be blindly transcribed into the trace.

I haven't thought this through completely, other than to note that the complete hydration method may somewhat contradict this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be better to have some explicit topological sorting mechanism

That's pretty much what hash_ptr_val does before calling hash_ptr_val_unsafe. So we can just call hash_ptr on specific pointers if we don't want to call hydrate_z_cache

self.dehydrated.load().len() < 443
} else {
// release mode
self.dehydrated.load().len() < 2497
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate these numbers.

Copy link
Contributor

@porcuquine porcuquine left a comment

Choose a reason for hiding this comment

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

This looks generally good to me.

@arthurpaulino arthurpaulino added this pull request to the merge queue Apr 17, 2024
Merged via the queue into main with commit bb04f94 Apr 17, 2024
20 of 22 checks passed
@arthurpaulino arthurpaulino deleted the ap/store-core branch April 17, 2024 23:13
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.

A better design for Z data
2 participants