Use IPLD-prime: target merge branch #67

hannahhoward · 2021-03-12T01:54:38Z

superseeds #64

switch to go-codec-dagpb
switch to go-ipld-prime linksystem branch

node.go

warpfork · 2021-08-03T18:52:17Z

Probably known, but noting out loud: this needs a rebase onto master (or some manual merging, owner's choice).

However, it doesn't look like it should be too nasty.

most of the changes are to CI infra, which hopefully won't conflict in interesting ways.
chore: downgrade blockstore #72 might be a little scary (it downgrades a dep?)... but it looks like the deps it changed are not changed for load-bearing reasons in this PR (afaict), so while it creates a conflict in go.mod, hopefully it's just a manual resolve in that file only.

merkledag.go

node.go

prime.go

warpfork · 2021-08-04T14:06:27Z

I think I'm mostly okay with this?

Per reasoning in the big review of repos and goalposts for this mergeparty I made over in go-path, the aim here is moving forward, and getting new code to do more load-bearing... and while focusing on that, we are conserving scope by leaving stuff in this repo as facades, and are specifically not polishing these APIs in any way. So the result is not going to be pretty, yet. And that's okay. We're just trying to be in a better place for the next work cycle.

AFAICT, checklist to get this ready for mergeparty:

Rebase onto master (or merge master in and resolve).
Some kind of doc comment on ProtoNode type to disambiguate vs the other similarly-named types in other repos, please
Should we... update the README to talk about alternatives folks should look to? Or does that remain unclear? (I just now noticed how much of it is a TODO section already, and it does even already say "deprecate this package"(!) so we can... keep with tradition, heh.)

.travis.yml

aschmahmann

Not many blocking issues, but this code is fairly tricky so I've asked for clarification in a number of places. Also, we need to rebase on master since we're out of date

coding.go

coding_test.go

merkledag.go

node.go

node_test.go

raw.go

Fix functionality for decode function -- was using incorrect decoder and not handling maybe cases

Points worth noting: 1) go-ipld-prime-proto contains a raw codec, but go-codec-dagpb does not. We're adding one such codec to go-ipld-prime, so for now, just add one here to keep the tests passing. 2) dagpb uses a slightly different schema. In particular, the Data field is optional, but a link's Hash is not. 3) As per the dag-pb spec, and following the above, links must contain hashes. This required updating a number of tests. Note that TestStableCID gets its CID changed because of this. go-ipld-prime-proto arrives at the same new CID with the new node. dagutils.TestInsertNode still fails, because it arrives at a different CID hash, but it's not clear why just yet.

to make merkeldag work with linksystem, update ipld-prime, go-codec-dagpb, and ipld-legacy

Allocate all the underlying links at once. This means one alloc per call, instead of one alloc per link. name old time/op new time/op delta Roundtrip-8 5.34µs ± 1% 5.20µs ± 1% -2.61% (p=0.002 n=6+6) name old alloc/op new alloc/op delta Roundtrip-8 7.50kB ± 0% 7.44kB ± 0% -0.85% (p=0.002 n=6+6) name old allocs/op new allocs/op delta Roundtrip-8 148 ± 0% 139 ± 0% -6.08% (p=0.002 n=6+6)

It's unclear to me why we did the Bytes+Cast dance for each cid.Cid in the links list. Perhaps to do a copy, but that wouldn't make sense as dagpb only reads the CIDs when marshaling. We can use the same Cid value directly, which shaves off a significant amount of allocation overhead. name old time/op new time/op delta Roundtrip-8 5.20µs ± 1% 4.81µs ± 0% -7.53% (p=0.004 n=6+5) name old alloc/op new alloc/op delta Roundtrip-8 7.44kB ± 0% 7.14kB ± 0% -4.09% (p=0.002 n=6+6) name old allocs/op new allocs/op delta Roundtrip-8 139 ± 0% 119 ± 0% -14.39% (p=0.002 n=6+6)

Cutting out the bytes.Buffer middleman saves a non-trivial amount of work, it turns out. name old time/op new time/op delta Roundtrip-8 3.24µs ± 3% 4.07µs ± 0% +25.72% (p=0.002 n=6+6) name old alloc/op new alloc/op delta Roundtrip-8 4.94kB ± 0% 6.38kB ± 0% +29.13% (p=0.002 n=6+6) name old allocs/op new allocs/op delta Roundtrip-8 109 ± 0% 103 ± 0% -5.50% (p=0.002 n=6+6)

hannahhoward · 2021-08-06T06:29:47Z

@aschmahmann I believe I've addressed most comments and this is ready for re-review

aschmahmann · 2021-08-11T17:37:54Z

node.go

 func (n *ProtoNode) UnmarshalJSON(b []byte) error {
 	s := struct {
-		Data  []byte       `json:"data"`
-		Links []*ipld.Link `json:"links"`
+		Data  []byte         `json:"data"`
+		Links []*format.Link `json:"links"`
 	}{}


@warpfork @rvagg @willscott does this need to be changed to Data and Links? Perhaps relatedly will this code be used anywhere anymore?

Use IPLD-prime: target merge branch This commit was moved from ipfs/go-merkledag@8794146

hannahhoward mentioned this pull request Mar 12, 2021

Use ipld-prime for encode & decode in DAG Service, construct ipld-prime compatible nodes #64

Closed

aschmahmann mentioned this pull request Apr 16, 2021

go-path integrated ipfs/kubo#8028

Merged

aschmahmann reviewed Apr 19, 2021

View reviewed changes

node.go Show resolved Hide resolved

hannahhoward mentioned this pull request Jul 22, 2021

IPLD Prime In IPFS: Target Merge Branch ipfs/kubo#7976

Merged

48 tasks

warpfork reviewed Aug 4, 2021

View reviewed changes

merkledag.go Show resolved Hide resolved

warpfork reviewed Aug 4, 2021

View reviewed changes

node.go Outdated Show resolved Hide resolved

warpfork reviewed Aug 4, 2021

View reviewed changes

prime.go Show resolved Hide resolved

aschmahmann reviewed Aug 5, 2021

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

aschmahmann requested changes Aug 5, 2021

View reviewed changes

hannahhoward and others added 14 commits August 5, 2021 22:30

feat(merkledag): use ipld-prime for encode & decode in DAG Service

4a93571

feat(coding): switch to qp for node building

a289dd4

fix(coding): fix decode funcs

485193b

Fix functionality for decode function -- was using incorrect decoder and not handling maybe cases

feat(deps): update go-ipld-legacy

de3fa87

use codec/raw

6eb498e

nil data shouldn't be encoded

d6ea3ca

feat(merkledag): update deps for linksystem

c29f538

to make merkeldag work with linksystem, update ipld-prime, go-codec-dagpb, and ipld-legacy

feat(deps): update to ipld trusted store

49c247e

add a PBNode roundtrip benchmark

53cb8e8

style(lint): fix lint errors

d863a07

hannahhoward force-pushed the feat/ipld-in-ipfs branch from f034e22 to d863a07 Compare August 6, 2021 05:43

fix(merkledag): address PR comments

1c8d118

fix(go.mod): go mod tidy

4c7b458

style(comment): add explanatory comment on test cid

9863085

aschmahmann approved these changes Aug 11, 2021

View reviewed changes

style(comments): add blank import comment

f0d2ffb

aschmahmann reviewed Aug 11, 2021

View reviewed changes

fix(deps): update to tagged dependencies

ed0a87b

willscott merged commit 8794146 into master Aug 12, 2021

aschmahmann mentioned this pull request Aug 23, 2021

Release v0.10 ipfs/kubo#8176

Closed

62 tasks

Jorropo pushed a commit to ipfs/boxo that referenced this pull request Mar 15, 2023

Merge pull request ipfs/go-merkledag#67 from ipfs/feat/ipld-in-ipfs

275d083

Use IPLD-prime: target merge branch This commit was moved from ipfs/go-merkledag@8794146

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use IPLD-prime: target merge branch #67

Use IPLD-prime: target merge branch #67

hannahhoward commented Mar 12, 2021

warpfork commented Aug 3, 2021

warpfork commented Aug 4, 2021

aschmahmann left a comment •

edited

Loading

hannahhoward commented Aug 6, 2021

aschmahmann Aug 11, 2021

Use IPLD-prime: target merge branch #67

Use IPLD-prime: target merge branch #67

Conversation

hannahhoward commented Mar 12, 2021

warpfork commented Aug 3, 2021

warpfork commented Aug 4, 2021

aschmahmann left a comment • edited Loading

Choose a reason for hiding this comment

hannahhoward commented Aug 6, 2021

aschmahmann Aug 11, 2021

Choose a reason for hiding this comment

aschmahmann left a comment •

edited

Loading