refactor(hamt): remove child interface from hamt package #30

overbool · 2018-10-09T07:52:15Z

fixes #28

schomatis

Great!! This is certainly looking much nicer. Just seeing the diversity in how we were sometimes referring to childs and others to shardValules to actually mean the same thing (now unified as Shard) is a good indication I think that we're in the right path.

Left some notes, don't take them as strong directives but rather as suggestions to think about. I still don't have a clear picture of what the final state of this package should look like and it's something that will need some more thought and discussion, some of my comments may be way off so feel free to point out whatever doesn't sound good to you.

schomatis · 2018-10-10T02:37:39Z

hamt/hamt.go

+	shardValueNode
+)
+
+func (ds *Shard) nodeType() nodeType {


I really like encapsulating this logic, ideally we are unifying both types to make their manipulation as transparent as possible so if we do indeed need this function it should be called as little as possible.

@schomatis I have a small thought about this: Shard only have 2 type nodes: Shard node and Value node, so maybe we can only use the follow function:

func (ds *Shard) isValueNode() bool { if ds.key != "" && ds.val != nil { return true } return false }

And then we can replace switch logic with if ... else.

WDYT?

Yeeees!! Definitely that is conceptually much clearer, since we only care about knowing where does our directory stop and the rest of the DAG hierarchy continues, that makes much sense.

My main concern with the "node" terminology here is that, although conceptually from the UnixFS layer perspective this is another (separate) node (a leaf node in the trie), actually it's being encoded inside its parent (internal) node in the DAG layer (i.e., that node is not stored separately in the DAG service). But I guess that is a correct abstraction, this collapse at the DAG level (for performance reasons) doesn't need to be complicating the logic here.

schomatis · 2018-10-10T02:42:30Z

hamt/hamt.go

 	lnk2 := *lnk
-	return &shardValue{
+	return &Shard{


We would need to call makeShard first to properly initialize the Shard structure and later load its key/value pair.

schomatis · 2018-10-10T02:46:38Z

hamt/hamt.go

@@ -219,6 +218,10 @@ func hash(val []byte) []byte {
 // Label for Shards is the empty string, this is used to differentiate them from
 // value entries
 func (ds *Shard) Label() string {
+	nodeType := ds.nodeType()


I think we could just always return the ds.key attribute here (without calling nodeType, in an effort to distantiate this logic from the shard/shard-value conflict), an internal node should have an empty string anyway.

schomatis · 2018-10-10T02:55:25Z

hamt/hamt.go

-	ds.children = append(ds.children[:i], append([]child{sv}, ds.children[i:]...)...)
-	ds.nd.SetLinks(append(ds.nd.Links()[:i], append([]*ipld.Link{nil}, ds.nd.Links()[i:]...)...))
+	ds.children = append(ds.children[:i], append([]*Shard{sv}, ds.children[i:]...)...)
+	ds.nd.SetLinks(append(ds.nd.Links()[:i], append([]*ipld.Link{lnk}, ds.nd.Links()[i:]...)...))


This does make sense to me but I'd prefer having the conservative approach of avoiding modifications in behavior as much as possible since this does not directly relate to the child interface. How we handle DAG links and the proto-node cache will be reviewed and refactored in another (separate) issue/PR.

schomatis · 2018-10-10T03:04:32Z

hamt/hamt.go

@@ -380,7 +388,7 @@ func (ds *Shard) rmChild(i int) error {
 	return nil
 }

-func (ds *Shard) getValue(ctx context.Context, hv *hashBits, key string, cb func(*shardValue) error) error {
+func (ds *Shard) getValue(ctx context.Context, hv *hashBits, key string, cb func(*Shard) error) error {


Note to self: this seems like another great example of where we could use the Walker, generalizing the Shard as just another type of node in a DAG, where we could call its Iterate method. But, as previously suggested, this may be an unnecessary over-complication. I just don't like seeing the same recursive/switch logic over and over everywhere in the code base.

I think this is actually the Search function from Walker. It's making a decision about which child to traverse next based on the files content hash.

schomatis · 2018-10-10T03:08:58Z

hamt/hamt.go

@@ -471,20 +480,22 @@ func makeAsyncTrieGetLinks(dagService ipld.DAGService, onShardValue func(*shardV
 	}
 }

-func (ds *Shard) walkTrie(ctx context.Context, cb func(*shardValue) error) error {
+func (ds *Shard) walkTrie(ctx context.Context, cb func(*Shard) error) error {


I'll raise a separate issue about this but we definitely need to unify this function with getValue.

my belief is getValue is a search -- it returns a single value (through a callback which this seems odd given you only want a single value), making a decision at every layer of the tree based on the file's content hash-- while walkTrie is an iteration (it traverses every node) -- so they're not the same exactly. But of course these are both the two functions implemented by your IPLD walker. So they could potentially both be replaced easily with your IPLD walker.

Also though, I am not exactly sure what context walkTrie will really get called instead of the async walker. Currently EnumLinks is an async walker (calls makeAsyncWalkTrieGetLinks) and ForEachLink is synchronous (calls walkTrie) -- but I don't know if ForEachLink is called by anything at this point. The other difference is they the synchronous walk is in order traversal while async makes no such gaurantees.

schomatis · 2018-10-10T03:21:19Z

hamt/hamt.go

 	ds.children[i] = c
 }

 // Link returns a merklelink to this shard node
 func (ds *Shard) Link() (*ipld.Link, error) {
+	nodeType := ds.nodeType()


Link is only called inside the Node method (hiding an implicit recursion) so we can just move this logic there and remove this function. However, if you think that it will generate too much confusion at this point in the PR we can iterate on it later.

@schomatis If we move this Link() inside the Node(), the Node() function may be too long.

Agreed, we can leave this for another PR.

hannahhoward

Looks good, certainly an improvement.

hannahhoward · 2018-10-15T15:18:02Z

hamt/hamt.go

@@ -471,20 +480,22 @@ func makeAsyncTrieGetLinks(dagService ipld.DAGService, onShardValue func(*shardV
 	}
 }

-func (ds *Shard) walkTrie(ctx context.Context, cb func(*shardValue) error) error {
+func (ds *Shard) walkTrie(ctx context.Context, cb func(*Shard) error) error {


my belief is getValue is a search -- it returns a single value (through a callback which this seems odd given you only want a single value), making a decision at every layer of the tree based on the file's content hash-- while walkTrie is an iteration (it traverses every node) -- so they're not the same exactly. But of course these are both the two functions implemented by your IPLD walker. So they could potentially both be replaced easily with your IPLD walker.

Also though, I am not exactly sure what context walkTrie will really get called instead of the async walker. Currently EnumLinks is an async walker (calls makeAsyncWalkTrieGetLinks) and ForEachLink is synchronous (calls walkTrie) -- but I don't know if ForEachLink is called by anything at this point. The other difference is they the synchronous walk is in order traversal while async makes no such gaurantees.

schomatis

LGTM (modulo those two nits).

schomatis · 2018-10-15T16:55:02Z

hamt/hamt.go

@@ -39,13 +39,20 @@ const (
 	HashMurmur3 uint64 = 0x22
 )

+func (ds *Shard) isValueNode() bool {
+	if ds.key != "" && ds.val != nil {


nit: you can just return this comparison.

@schomatis fixed.

schomatis · 2018-10-15T16:57:37Z

hamt/hamt.go

-}
+func (ds *Shard) makeShardValue(lnk *ipld.Link) *Shard {
+	lnk2 := *lnk
+	s, _ := makeShard(ds.dserv, ds.tableSize)


Let's check for errors just in case.

@schomatis
Did you mean that we should add panic here?
IMO, I think the err always will be nil , because the ds.tableSize is a correct size.

I would add an error check and then have makeShardValue be able to return an error, then add error checks on the two places it's called, which are both in functions which already pass on errors.

@hannahhoward yeah, a good advise

@hannahhoward @schomatis fixed

hannahhoward · 2018-10-16T08:42:41Z

@schomatis are your changes blocking? (i.e. can we merge and fix afterward or you want to make sure @overbool fixes them first?)

schomatis · 2018-10-16T12:55:50Z

Really nice work!

overbool requested a review from schomatis as a code owner October 9, 2018 07:52

overbool force-pushed the refactor/remove-child-from-hamt branch from 20e5cd5 to ae9cb5f Compare October 9, 2018 12:29

schomatis reviewed Oct 10, 2018

View reviewed changes

overbool force-pushed the refactor/remove-child-from-hamt branch from ae9cb5f to 9fdf744 Compare October 10, 2018 07:01

refactor(hamt): remove child interface from hamt pkg

2d832ee

overbool force-pushed the refactor/remove-child-from-hamt branch from 9fdf744 to 2d832ee Compare October 10, 2018 07:55

overbool changed the title ~~[WIP] refactor(hamt): remove child interface from hamt package~~ refactor(hamt): remove child interface from hamt package Oct 10, 2018

hannahhoward approved these changes Oct 15, 2018

View reviewed changes

schomatis approved these changes Oct 15, 2018

View reviewed changes

schomatis mentioned this pull request Oct 15, 2018

ipld/unixfs/hamt: Encapsulate and document maxpadlen logic ipfs/boxo#388

Open

overbool added 2 commits October 16, 2018 17:03

fix(hamt): modify isValueNode

ed161b8

fix(hamt): add error in makeShardValue

37ff336

schomatis merged commit 3cc73ee into ipfs:master Oct 16, 2018

This was referenced Oct 16, 2018

HAMT: evaluate using the DAG walker in getValue #37

Closed

HAMT: review getValue and walkTrie #38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(hamt): remove child interface from hamt package #30

refactor(hamt): remove child interface from hamt package #30

overbool commented Oct 9, 2018 •

edited by hannahhoward

Loading

schomatis left a comment •

edited

Loading

schomatis Oct 10, 2018

overbool Oct 10, 2018

schomatis Oct 10, 2018

schomatis Oct 10, 2018

schomatis Oct 10, 2018

schomatis Oct 10, 2018

schomatis Oct 10, 2018

schomatis Oct 10, 2018

hannahhoward Oct 15, 2018

schomatis Oct 10, 2018

hannahhoward Oct 15, 2018

schomatis Oct 10, 2018

overbool Oct 10, 2018

schomatis Oct 10, 2018

hannahhoward left a comment

hannahhoward Oct 15, 2018

schomatis left a comment

schomatis Oct 15, 2018

overbool Oct 16, 2018

schomatis Oct 15, 2018

overbool Oct 16, 2018

hannahhoward Oct 16, 2018

overbool Oct 16, 2018

overbool Oct 16, 2018

hannahhoward commented Oct 16, 2018

schomatis commented Oct 16, 2018

refactor(hamt): remove child interface from hamt package #30

refactor(hamt): remove child interface from hamt package #30

Conversation

overbool commented Oct 9, 2018 • edited by hannahhoward Loading

schomatis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahhoward left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schomatis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahhoward commented Oct 16, 2018

schomatis commented Oct 16, 2018

overbool commented Oct 9, 2018 •

edited by hannahhoward

Loading

schomatis left a comment •

edited

Loading