Closed schomatis closed 6 years ago
/cc @overbool You said you wanted a challenge. :grin:
@schomatis OK, I like it. 😁
@schomatis I have a question about removing shardValue
, is there any obvious benefit to doing this?
is there any obvious benefit to doing this?
Most definitely not. This won't fix any bug, add any cool new feature, improve the performance or reduce the memory/disk footprint.
But if the objective was obvious I wouldn't deem this a challenge in the first place ;)
This issue is part of an effort to simplify the logic of the hamt
package (as described in ipfs/boxo#387). My reasoning is the following: if you take a look at the history of the hamt.go
file you'll see that only two developers contributed any meaningful logic to it, @whyrusleeping (the original creator) that is now not involved in the day-to-day operations of go-ipfs
anymore and @Stebalien who is basically in charge of any piece of code written in Go (which is a lot) so we can't depend on them to review, explain, fix and maintain this code (which we'll be relying on even more in the future to improve IPFS performance and extend its use). This translates to situations like for example https://github.com/ipfs/go-unixfs/pull/19#discussion_r221825922 where new features being added to the code take much longer than expected because there's parts of the code that we're not really sure how they work and reviewers (like me) need to take a much longer look at it and think much more about it to understand what's going on. The complement to that situation is that also new developers (like you) will think twice before diving into that part of the code because of the time and effort that will entail (and probably just abandon that endeavor altogether).
That being said, don't feel obligated to take this one on, if you find other issues to work on that you feel will be more rewarding in terms of what you can learn from them or that just seem more interesting to you, go for it! This can be handled by someone else or you yourself can come back to it later on if you feel like it.
@schomatis I see your point and agree with you. I just want to discuss it with you and no other meanings.
Therefore, I am still willing to do it.
I just want to discuss it with you and no other meanings.
Np, it was a valid question since this is not a trivial task and we need to be sure if this is adding value to the code before investing (before you invest) too much time on it, so if you think of a different approach worth discussing feel free to raise the issue here and we can talk about it.
Therefore, I am still willing to do it.
Great! Ping me if you have any trouble, the initial steps are not complete and you'll probably encounter collateral issues while removing this interface.
Background: https://github.com/ipfs/boxo/issues/387.
(This is not completely thought out but a broad description of what we need to do. Further discussion and testing will probably be needed.)
The idea is to eliminate the interface and unify
Shard
andshardValue
. Make it evident that values (links to entries in the directory) are stored at the leaf nodes with only one value per node. TheShard
at the moment contains many values but that is just because it encodes the child nodes itself as a performance improvement to avoid requesting more nodes at the DAG layer. The performance should be maintained but it needs to be clarified, the shard/shard-value logic should be encapsulated in theloadChild
(and related functions).Concretely:
child
interface and theshardValue
.children
node a slice ofShard
.Shard
to save the value it was once stored in theshardValue
, that is, thekey
/value
pair (that may need to be renamed).switch
logic that handled theShard
andshardValue
separately.loadChild
to also createShard
s instead ofshardValue
s.