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

Optimization 1 for switchToBasic function in unsharding PR #101

Merged
merged 1 commit into from
Aug 3, 2021
Merged

Optimization 1 for switchToBasic function in unsharding PR #101

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions io/directory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,17 +198,28 @@ func (d *BasicDirectory) SetCidBuilder(builder cid.Builder) {
// AddChild implements the `Directory` interface. It adds (or replaces)
// a link to the given `node` under `name`.
func (d *BasicDirectory) AddChild(ctx context.Context, name string, node ipld.Node) error {
link, err := ipld.MakeLink(node)
if err != nil {
return err
}

return d.addLinkChild(ctx, name, link)
}

// addLinkChild adds the link as an entry to this directory under the given
// name. Plumbing function for the AddChild API.
func (d *BasicDirectory) addLinkChild(ctx context.Context, name string, link *ipld.Link) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the name seems redundant given that ipld.Link already includes a name.

(I mean, ideally we could just use a CID... but we need the size because dagpb).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first version was actually just the link argument with the name packed inside but looking at how we manipulate links in go-merkledag it seemed like the idiomatic way to go was to maniuplate that separately. Even internally here in go-unixfs we handle the name separately.

I totally agree with you and the current way seems a bit counter-intuitive to me so happy to revert to my previous version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, leave it. Ideally we'd just get rid of that type entirely but that's work for another day.

// Remove old link (if it existed; ignore `ErrNotExist` otherwise).
err := d.RemoveChild(ctx, name)
if err != nil && err != os.ErrNotExist {
return err
}

err = d.node.AddNodeLink(name, node)
err = d.node.AddRawLink(name, link)
if err != nil {
return err
}
d.addToEstimatedSize(name, node.Cid())
d.addToEstimatedSize(name, link.Cid)
return nil
}

Expand Down Expand Up @@ -398,22 +409,14 @@ func (d *HAMTDirectory) switchToBasic(ctx context.Context) (*BasicDirectory, err
basicDir.SetCidBuilder(d.GetCidBuilder())

d.ForEachLink(context.TODO(), func(lnk *ipld.Link) error {
node, err := d.dserv.Get(ctx, lnk.Cid)
if err != nil {
return err
}

err = basicDir.AddChild(ctx, lnk.Name, node)
err := basicDir.addLinkChild(ctx, lnk.Name, lnk)
if err != nil {
return err
}

return nil
})
// FIXME: The above was adapted from SwitchToSharding. We can probably optimize:
// 1. Do not retrieve the full node but create the link from the
// HAMT entry and add it to the BasicDirectory with a (new)
// plumbing function that operates below the AddChild level.
// 2. Do not enumerate all link from scratch (harder than previous
// item). We call this function only from the UpgradeableDirectory
// when we went below the threshold: the detection will be done through
Expand Down