Skip to content
This repository was archived by the owner on Jun 27, 2023. It is now read-only.

Remove the child interface from the hamt package #28

Closed
schomatis opened this issue Oct 4, 2018 · 6 comments · Fixed by #30
Closed

Remove the child interface from the hamt package #28

schomatis opened this issue Oct 4, 2018 · 6 comments · Fixed by #30
Labels
exp/expert Having worked on the specific codebase is important help wanted Seeking public contribution on this issue topic/technical debt Topic technical debt

Comments

@schomatis
Copy link
Contributor

Background: ipfs/boxo#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 and shardValue. Make it evident that values (links to entries in the directory) are stored at the leaf nodes with only one value per node. The Shard 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 the loadChild (and related functions).

Concretely:

  • Remove the child interface and the shardValue.
  • Make the children node a slice of Shard.
  • Extend Shard to save the value it was once stored in the shardValue, that is, the key/value pair (that may need to be renamed).
  • Unify every switch logic that handled the Shard and shardValue separately.
  • Modify the loadChild to also create Shards instead of shardValues.
@schomatis schomatis added help wanted Seeking public contribution on this issue topic/technical debt Topic technical debt status/ready Ready to be worked exp/expert Having worked on the specific codebase is important labels Oct 4, 2018
@schomatis
Copy link
Contributor Author

/cc @overbool You said you wanted a challenge. 😁

@overbool
Copy link
Contributor

overbool commented Oct 4, 2018

@schomatis OK, I like it. 😁

@overbool
Copy link
Contributor

overbool commented Oct 9, 2018

@schomatis I have a question about removing shardValue, is there any obvious benefit to doing this?

@schomatis
Copy link
Contributor Author

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 #19 (comment) 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.

@overbool
Copy link
Contributor

overbool commented Oct 9, 2018

@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.

@schomatis
Copy link
Contributor Author

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.

@hannahhoward hannahhoward added status/in-progress In progress and removed status/ready Ready to be worked labels Oct 15, 2018
@ghost ghost removed the status/in-progress In progress label Oct 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/expert Having worked on the specific codebase is important help wanted Seeking public contribution on this issue topic/technical debt Topic technical debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants