perf: make this._json calculation lazy #81
Conversation
Codecov Report
@@ Coverage Diff @@
## master #81 +/- ##
==========================================
+ Coverage 90.87% 91.04% +0.16%
==========================================
Files 13 13
Lines 274 268 -6
==========================================
- Hits 249 244 -5
+ Misses 25 24 -1
Continue to review full report at Codecov.
|
| @@ -78,6 +77,7 @@ | |||
| "ipfs-block-service": "~0.14.0", | |||
| "ipfs-repo": "~0.22.1", | |||
| "lodash": "^4.17.10", | |||
| "multihashes": "~0.4.13", | |||
There was a problem hiding this comment.
I think multihashes is no needed after this very good change.
There was a problem hiding this comment.
It's still being used in a test which is why it's moved from dependencies to devDependencies.
|
@achingbrain @vmx @diasdavid as per #80 (comment) are there any objections to providing a Ideally we'd remove the I'm already normalising all IPLD nodes to have a |
Fine by me. It'd be good to get people using CIDs instead of multihashes as they aren't the same thing in this brave new CIDv1 world. Would you have it return the object or the buffer? |
|
I had imagined it'd return the buffer to be a more minimal change. In the common case of cidv0 it'd mean the usage site just has to rename the property. I do have to upgrade the buffers to CID objects all over the place right now, so it'd help to use CID objects over buffers in the object representations, but we would have to review all the IPLD node types. I think we should do the smaller change now... renaming multihash to cid but have it still return a Buffer. Then as a more awesome endeavour, we should figure out if all links in all nodes should be represented as a CID object rather than a Buffer. |
|
I think it’d be better to just have .cid return the object as, like you say, people convert them to objects anyway, particularly if you then want to work with the ipld APIs at all, which you probably want to if you are messing with CIDs in the first place.
I’d leave .multihash returning a Buffer in place for backwards compatibility and perhaps consider getting it to print a warning and removing it at some point in future.
Otherwise we make one breaking change to remove .multihash and add .cid, then potentially another to go from Buffer to Object. We can swerve all that disruption by just going for the useful API straight off.
What do you think?
… On 2 Aug 2018, at 11:31, Oli Evans ***@***.***> wrote:
I had imagined it'd return the buffer to be a more minimal change. In the common case of cidv0 it'd mean the usage site just has to rename the property.
I do have to upgrade the buffers to CID objects all over the place right now, so it'd help to use CID objects over buffers in the object representations, but we would have to review all the IPLD node types.
I think we should do the smaller change now... renaming multihash to cid but have it still return a Buffer. Then as a more awesome endeavour, we should figure out if all links in all nodes should be represented as a CID object rather than a Buffer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Compelling argument @achingbrain. I don't have a feel for the direction the IPLD interfaces are supposed to be moving in, I had assumed that using Buffers all over the place was deliberate and I just hadn't found the repo that explained it yet, but I do agree it doesn't make for an intuitive API for people that want to traverse IPLD nodes. Vaguely related to this is the discussion around what is the standard pattern for the IPLD transform that we apply when representing entities from other systems as objects ipld/js-ipld-ethereum#25 |
|
@achingbrain @olizilla It's funny that the CID object vs. Buffer comes up as @mikeal and I were discussing the same thing during the DWeb Summit. Combining that discussion with yours above I'd say the outcome would be that we have a new Please everyone mentioned in this comment give a thumb up if you agree, or post a comment if you don't. It'll then be a separate change, independent of this one. Update: Reference to the CID discussions: ipld/ipld#44 |
I've been doing some profiling of js-ipfs and where there are large amounts of DAG operations being performed (importing all of npm to ipfs, for example) we spend about 1.8-2.5% of our time in the creation of this._json objects, even though they are not always used, so this PR makes them be created when first requested. It also passes the `this._json` object through `Object.freeze` becase otherwise we expose a mutable object to the world - it seems odd that we've taken pains to ensure that the properties of a DAGNode/DAGLink are immutable, then expose objects whose properties are not immutable. Finally it makes more use of the `cids` module to parse CIDs from multihashes as the `cids` module respects CIDv1 versions/codecs if present whereas the `multihashes` module does not and is only really suitable for use with CIDv0.
achingbrain
force-pushed
the
make-json-serialization-lazy
branch
from
6c8ed4d to
d9e0e55
Compare
|
This is the sort of thing I am seeing while importing npm to IPFS without this PR:
The other really long flat bar is the garbage collector. Flame graph from appmetrics-dash. |
|
Can it be merged & released? |
|
@achingbrain Sorry this somehow slipped my attention. I've also done a release. |
Relaxes the constraints from #81 a little as it's caused quite a lot of tests to break. We still have an immutable copy in the object but any objects returned will be mutable.
Relaxes the constraints from #81 a little as it's caused quite a lot of tests to break. We still have an immutable copy in the object but any objects returned will be mutable.
...which was removed in ipld/js-ipld-dag-pb#81 License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>
This PR contains fixes for the test failures that are happening in master for `object.patch.rmLink`. I **think** this change ipld/js-ipld-dag-pb#80 made `DAGLink` validate the hash better and caused the failures to start. It also contains fixes for the `pinSet` module which was using the "private" `_multihash` property which was removed in ipld/js-ipld-dag-pb#81 and released 2 days ago - don't use private APIs kids. License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>
Implements the change proposed in #81 (comment)
Implements the change proposed in #81 (comment)
I've been doing some profiling of js-ipfs and where there are large amounts of DAG operations being performed (importing all of npm to ipfs, for example) we spend about 1.8-2.5% of our time in the creation of this._json objects, even though they are not always used, so this PR makes them be
created when first requested.
It also passes the
this._jsonobject throughObject.freezebecause otherwise we expose a mutable object to the world - it seems odd that we've taken pains to ensure that the properties of a DAGNode/DAGLink are immutable, then expose objects whose properties are not immutable.Finally it makes more use of the
cidsmodule to parse CIDs from multihashes as thecidsmodule respects CIDv1 versions/codecs if present whereas themultihashesmodule does not and is only really suitable for use with CIDv0.