feat: switch to HAMT based on size #91
Conversation
schomatis
force-pushed
the
schomatis/feat/size-switch-hamt
branch
from
53f4e21
to
7237e57
Compare
| // HAMTShardingSize option. We maintain this value even if the | ||
| // HAMTShardingSize is off since potentially the option could be activated | ||
| // on the fly. | ||
| estimatedSize int |
There was a problem hiding this comment.
Let's:
- Document the exact algorithm.
- Maybe call this "linkdataSize", or something equally inscrutable.
This "estimate" needs to accurately follow the algorithm to achieve convergence. While this is more of a "SHOULD" from a spec standpoint, I'd like to be as accurate as possible.
There was a problem hiding this comment.
The full documentation is in the exported HAMTShardingSize (referenced here) which is what the user has access to; this is just an internal cache. I'd like to retain this name (or something similar to this effect) to better represent what we want to achieve (estimation of directory size) rather than the how (traversing links).
Also not following the talk about a spec (even if figuratively) and a reference to an algorithm which I'm not sure what part of the code it is designating (is it the addToEstimatedSize/removeFromEstimatedSize functions?).
All this said, if this is a blocking change feel free to push any documentation you want or rename however you prefer. Not very opinionated on this and it won't change how this PR works anyway.
There was a problem hiding this comment.
I guess I just want something to indicate that this isn't just some random estimate that someone can just change, but that's not critical or merge blocking.
| // HAMTShardingSize option. We maintain this value even if the | ||
| // HAMTShardingSize is off since potentially the option could be activated | ||
| // on the fly. | ||
| estimatedSize int |
There was a problem hiding this comment.
I guess I just want something to indicate that this isn't just some random estimate that someone can just change, but that's not critical or merge blocking.
io/directory.go
Outdated
|
|
||
| func (d *BasicDirectory) addToEstimatedSize(name string, linkCid cid.Cid) { | ||
| d.estimatedSize += estimatedLinkSize(name, linkCid) | ||
| // FIXME: Ideally we may want to track the Link size as well but it is |
There was a problem hiding this comment.
Let's remove this FIXME, we don't want someone to actually come along and fix this in code (it's a spec change, effectively.
Replace global binary
UseHAMTShardingoption with threshold (also global) oneHAMTShardingSizethat triggers the switch to HAMT based on estimated directory size.Toward ipfs/go-mfs#87.