-
Notifications
You must be signed in to change notification settings - Fork 314
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(storage-proofs): partial caching for SDR #1163
Conversation
fdd041a
to
9f2ee80
Compare
3d69ca2
to
0536135
Compare
0536135
to
f6b3909
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.
Looks good. Is the CI error real?
I believe it is a bug in the tests, but will investigate |
f6b3909
to
5feb0d0
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.
One blocking request to make the cache size a setting.
Otherwise looks good, once CI is fixed. If fixing CI means changing the test, I'd like to see what's going on before we merge.
const NODE_GIB: u32 = (1024 * 1024 * 1024) / NODE_SIZE as u32; | ||
|
||
// Number of nodes to be cached in memory | ||
const DEFAULT_CACHE_SIZE: u32 = 2048; |
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 should go in settings.rs
.
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.
done
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.
Thank you. If there are constraints on this value we know need to be honored, please document those too (in settings.rs
). Does it need to evenly divide the number of nodes, for example? People should be allowed to shoot themselves in the foot, but we should warn them where the foot is, too.
no mem cache anymore?I think this will affect multiple sector sealing speeds.mem cache still necessary |
there is still a mem cache, it is simply done differently |
precommit_phase1
by 56GiB for 3i2GB sectors, when using max caching/var/tmp/filecoin-parents
for nowBased on the ideas in #1135