Refactor to refmt #30

dignifiedquire · 2017-11-13T20:51:42Z

Unmarshal uses refmt and passes existing tests
Marshal uses refmt and passes existing tests:
- waiting on response in panic("todo") when marshalling cbor tags polydawn/refmt#9 - fixed through not using pointers
- waiting on response in Emptish objects fail to roundtrip cbor marshal & unmarshal polydawn/refmt#10
- waiting on response in Nested Cbor Tags are not unmarshalled properly polydawn/refmt#13
Cleanup

whyrusleeping · 2017-11-13T22:32:12Z

node.go

-	cid   *cid.Cid
+	err = cbor.UnmarshalAtlased(b, &m, cborAtlas)
+	if err != nil {
+		fmt.Printf("bytes: %s\n", hex.EncodeToString(b))


btw, you can use %x instead of %s to automatically encode to hex.

sweet, thank you <3

dignifiedquire · 2017-11-16T18:03:22Z

More tests are passing with the latest fixes + latest refmt. Some more tests to fix before we are ready.

whyrusleeping · 2017-12-12T23:22:28Z

I actually think that something is broken on master. In TestMarshalRoundtrip, the object that gets created (on master) looks like this:

why@manwe /t/go-ipld-cbor> echo -n a36362617a82d82a5823001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505ed82a58230012205773487d221545d26fd0f57fdb3a7d986bc479a850d7b0d762e8c7f4772790a063666f6f636261726463617473a163717578d82a5823001220434be0538b88f385bdb9d62de34e296a0908c857a876aec09e1f52673596d897 | refmt cbor.hex=pretty
Map<len:3> {
        "baz": Array<len:2> [
                _tag:42_ 001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505e
                _tag:42_ 0012205773487d221545d26fd0f57fdb3a7d986bc479a850d7b0d762e8c7f4772790a0
        ]
        "foo": "bar"
        "cats": Map<len:1> {
                "qux": _tag:42_ 001220434be0538b88f385bdb9d62de34e296a0908c857a876aec09e1f52673596d897
        }
}

The string 'hello' is missing. This test started failing once we switched to refmt, and the created object looks like:

why@manwe ~/g/s/g/i/go-ipld-cbor> echo -n a46362617a82d82a5823001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505ed82a58230012205773487d221545d26fd0f57fdb3a7d986bc479a850d7b0d762e8c7f4772790a063666f6f636261726463617473a163717578d82a5823001220434be0538b88f385bdb9d62de34e296a0908c857a876aec09e1f52673596d8976568656c6c6fd82a5823001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505e | refmt cbor.hex=pretty
Map<len:4> {
        "baz": Array<len:2> [
                _tag:42_ 001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505e
                _tag:42_ 0012205773487d221545d26fd0f57fdb3a7d986bc479a850d7b0d762e8c7f4772790a0
        ]
        "foo": "bar"
        "cats": Map<len:1> {
                "qux": _tag:42_ 001220434be0538b88f385bdb9d62de34e296a0908c857a876aec09e1f52673596d897
        }
        "hello": _tag:42_ 001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505e
}

So while I was trying to avoid changing any tests, I think we actually should here.

whyrusleeping · 2017-12-13T22:37:34Z

Ignore my previous comment, it looks like @dignifiedquire added that hello key in earlier commits but didnt update the expected hash.

whyrusleeping · 2017-12-13T22:38:38Z

All thats left here is to add in the right sorting for maps to refmt for canonical cbor. I can manually make that change (and have locally), but would rather do it in a way thats flexible and have it officially be a part of refmt.

dignifiedquire · 2017-12-13T22:41:52Z

yeah that was probably some debugging tries that I forgot to clean up

…

On 13. Dec 2017, 23:38 +0100, Whyrusleeping ***@***.***>, wrote: All thats left here is to add in the right sorting for maps to refmt for canonical cbor. I can manually make that change (and have locally), but would rather do it in a way thats flexible and have it officially be a part of refmt. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

dignifiedquire · 2018-01-23T11:04:03Z

@whyrusleeping I think this is ready as soon as polydawn/refmt#19 is merged and we have a gx version of refmt.

whyrusleeping · 2018-01-24T01:33:40Z

Alright, we're ready to go here I think

dignifiedquire · 2018-02-19T13:43:39Z

@whyrusleeping is there a reason this is not merged yet?

whyrusleeping · 2018-02-21T00:09:00Z

@dignifiedquire mostly because we need to do the update into go-ipfs intentionally, and not have it get included arbitrarily with a bunch of other updates. This sort of thing is a higher risk change, and is something we want to test carefully and individually.

Stebalien · 2018-07-17T02:30:46Z

This is blocked until it stops deserializing CIDs as ~~*cid.Cid~~ cid.Cid and starts deserializing them to *cid.Cid.

whyrusleeping · 2018-07-17T03:24:15Z

@Stebalien cant we just change the castBytesToCid method to return a *cid.Cid?

Stebalien · 2018-07-17T23:30:33Z

@whyrusleeping no, this is a limitation of refmt as it stands (I've discussed it with @warpfork and I just filed an issue (polydawn/refmt#32) so we can track it).

We could fix all the CIDs up using traverse but that would mean an additional step.

My other open concern is that I'm not sure we really want to expose the RegisterCBORType function as it could screw with Resolve (maybe?).

Kubuxu · 2018-09-07T18:44:17Z

We also need: polydawn/refmt@2e9ba99

warpfork · 2018-09-09T13:59:17Z

I have attempted to unblock :)

Also, use a cloner instead of unmarshalling when wrapping an object.

Increasing `numWorkers` doesn't seem to help at all (not supprising). Really, we could probably drop to NumCPUs (instad of 2x that) and we'd still be fine.

More than that doesn't help. +1 helps for the case where a goroutine holding a worker gets unscheduled.

IIRC, pools are cleaned every GC cycle. However, by benchmarks aren't actually *faster* with this change so I figured we might as well go for it. Our issue here is really throughput, not latency, so even *if* these pools are cleaned on GC, it's still useful.

We don't need to mutate, just replace.

@warpfork

Make sure we *actually* serialize/deserialize. Addresses @warpfork's CR.

Stebalien · 2018-09-12T04:24:19Z

I've just rebased this on master in the refmt-rebased branch. Basically, we now just use cid.Cid (no pointers) so I got rid of all the "if we have a pointer do x, if we don't do y" stuff.

I've done a sanity check with the new uber-cool git range-diff master refmt refmt-rebased command but feel free to do the same.

@dignifiedquire please reset this branch to refmt-rebased when you're ready.

warpfork · 2018-09-16T07:17:46Z

What's the landing strategy going to be for this? IIUC, there's already releases of go-ipld-cbor cut from this branch being used in some other projects...

I guess gx snapshots also do preserve those, but ditching the original git commit hashes matching those releases would also seem pretty lossy.

Idea: do a giant git checkout master && git merge -s ours refmt to keep this history (but not apply the diff to master yet -- so there's no conflict resolution to do) and make it referenced from master so we can drop the branch ref... and then do another giant commit that's the rebased squash of this (with conflicts resolved).

Maybe that's unnecessary footwork, just floating the idea.

Stebalien · 2018-09-17T18:13:37Z

It's probably fine to drop the release commits, IMO. That shouldn't actually break anything due to how gx works.

dignifiedquire · 2018-09-17T18:35:53Z

@Stebalien maybe best you do what you want with the branch, I have a fork for the current version I need, so nothing should break.

…

On 17. Sep 2018, 20:13 +0200, Steven Allen ***@***.***>, wrote: It's probably fine to drop the release commits, IMO. That shouldn't actually break anything due to how gx works. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ghost assigned dignifiedquire Nov 13, 2017

ghost added the status/in-progress In progress label Nov 13, 2017

whyrusleeping reviewed Nov 13, 2017

View reviewed changes

ghost assigned whyrusleeping Dec 11, 2017

dignifiedquire changed the title ~~[WIP] Refactor to refmt~~ Refactor to refmt Feb 19, 2018

tobowers mentioned this pull request Jul 17, 2018

slow parallel processing #37

Closed

Stebalien mentioned this pull request Jul 17, 2018

Allow pointers when using UseTag polydawn/refmt#32

Open

Stebalien mentioned this pull request Aug 29, 2018

continuation of use string instead of struct backing ipfs/go-cid#71

Merged

ghost assigned Stebalien Sep 7, 2018

dignifiedquire added 7 commits September 11, 2018 21:07

feat: initial refactor to use refmt

9e7d558

fix copyObj

e14a4d9

no more panic todo

1d048f4

golint happy and some fixes

3c65d0f

more test cases

5106752

fix examples tests using latest refmt

699e7a0

use standard test script on travis

4982e8a

dignifiedquire and others added 17 commits September 11, 2018 21:16

cleanup code

e506e5e

some cleanup

f9b953e

fix json input with cids

e94e5a0

tests pass, but requires fix in refmt

eab8781

fix canonical struct encoding

fcd1522

fixes and updates

b29b7f4

update to latest cbor library syntax for sorting

573c793

cleanup errors and comments

ff21104

final import of refmt after fixes upstream

666b656

test: add failing test for nested structs

31243bb

update refmt

782a191

share a pool of (un)marshallers

679ed5f

Also, use a cloner instead of unmarshalling when wrapping an object.

add a parallel benchmark

cd13b8b

Increasing `numWorkers` doesn't seem to help at all (not supprising). Really, we could probably drop to NumCPUs (instad of 2x that) and we'd still be fine.

drop to NumCPU + 1 workers

ff48bd0

More than that doesn't help. +1 helps for the case where a goroutine holding a worker gets unscheduled.

replace SetAtlas method with a constructor

5896ec2

We don't need to mutate, just replace.

fix benchmarks

a1a7802

Make sure we *actually* serialize/deserialize. Addresses @warpfork's CR.

Stebalien force-pushed the refmt branch from cb4b1f0 to a1a7802 Compare September 19, 2018 03:03

Stebalien approved these changes Sep 19, 2018

View reviewed changes

Stebalien merged commit f8f4e93 into master Sep 19, 2018

ghost removed the status/in-progress In progress label Sep 19, 2018

Stebalien deleted the refmt branch September 19, 2018 03:43

rvagg mentioned this pull request Dec 8, 2021

Handling of undefined for old blocks ipld/js-dag-cbor#44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to refmt #30

Refactor to refmt #30

dignifiedquire commented Nov 13, 2017 •

edited

Loading

whyrusleeping Nov 13, 2017

dignifiedquire Nov 13, 2017

dignifiedquire commented Nov 16, 2017

whyrusleeping commented Dec 12, 2017

whyrusleeping commented Dec 13, 2017

whyrusleeping commented Dec 13, 2017

dignifiedquire commented Dec 13, 2017 via email

dignifiedquire commented Jan 23, 2018

whyrusleeping commented Jan 24, 2018

dignifiedquire commented Feb 19, 2018

whyrusleeping commented Feb 21, 2018

Stebalien commented Jul 17, 2018 •

edited

Loading

whyrusleeping commented Jul 17, 2018

Stebalien commented Jul 17, 2018

Kubuxu commented Sep 7, 2018

warpfork commented Sep 9, 2018

Stebalien commented Sep 12, 2018

warpfork commented Sep 16, 2018

Stebalien commented Sep 17, 2018

dignifiedquire commented Sep 17, 2018 via email

Refactor to refmt #30

Refactor to refmt #30

Conversation

dignifiedquire commented Nov 13, 2017 • edited Loading

whyrusleeping Nov 13, 2017

Choose a reason for hiding this comment

dignifiedquire Nov 13, 2017

Choose a reason for hiding this comment

dignifiedquire commented Nov 16, 2017

whyrusleeping commented Dec 12, 2017

whyrusleeping commented Dec 13, 2017

whyrusleeping commented Dec 13, 2017

dignifiedquire commented Dec 13, 2017 via email

dignifiedquire commented Jan 23, 2018

whyrusleeping commented Jan 24, 2018

dignifiedquire commented Feb 19, 2018

whyrusleeping commented Feb 21, 2018

Stebalien commented Jul 17, 2018 • edited Loading

whyrusleeping commented Jul 17, 2018

Stebalien commented Jul 17, 2018

Kubuxu commented Sep 7, 2018

warpfork commented Sep 9, 2018

Stebalien commented Sep 12, 2018

warpfork commented Sep 16, 2018

Stebalien commented Sep 17, 2018

dignifiedquire commented Sep 17, 2018 via email

dignifiedquire commented Nov 13, 2017 •

edited

Loading

Stebalien commented Jul 17, 2018 •

edited

Loading