Improve constructor performance

[dignifiedquire]

When I was doing some profiling I found out that if loading a lot of blocks from storage, new CID gets in to pretty hot code paths and starts becoming a bottleneck. Specifically the CID.isCID is not well optimized as it uses try catch to determine if we already have a CID

[daviddias]

Interesting, you mean that using a try/catch block is slower than? Perhaps returning a Boolean?

I'm also down to coalesce this two methods into one https://github.com/ipld/js-cid/blob/master/src/index.js#L220-L255

[realharry]

Hi, @dignifiedquire I'm interested in taking a look at this issue. Do you happen to have any particular use case(s) you have in mind? Like, maybe, a sample code that heavily calls new CID? (Or, I can just focus on the constructor without worrying about usages.) @diasdavid Considering both are public methods, do you have suggestions as to how to go about doing that? Clearly, (I presume) removing one or the other will potentially break dependent libs/apps. Is there a best practice for deprecating public methods? Or, we can optimize both separately without one method having to rely on another. Thanks.

[dignifiedquire]

The main usage I was looking at was when profiling js-ipfs as a whole. Especially when doing file transfers.

[realharry]

OK. thanks. Let me start from bottom up, and see if any change we make, if any, will make a difference in such a use case.

[realharry]

@dignifiedquire / @diasdavid,

Here's a quick update. I tried simply changing isCID() to return false/true directly instead of relying on error try/catch, and the new CID() was sped up about 3 ~ 4 times. On my laptop, running new CID() 10,000 times took about 70 ~ 75 ms with the current code. After the change, the time was about 17 ~ 20 ms. The interesting thing was that the time from the current code sometimes went up ~100 ms once in a while. (I'm suspecting it had something to do with gc. Creating Error objects on heap was probably taking bulk of the time.) Test env: Node.js version 6.11.5. Ubuntu Desktop 17.10.

I can look further, but it's clearly a low-hanging fruit which can show some benefits immediately. I can create a PR if you guys want to include this change. The only downside is that there is some code duplication. (Without knowing all the relevant code, I cannot make any type of aggressive optimizations.) Because of their internal logic, it's not easy to use a common/shared function, etc. (We can introduce an "error code", etc., but I think it's an overkill, possibly.)

Please let me know how you guys want to proceed.

[realharry]

I went ahead and created a PR #45 for your review. Thanks. ~h

[daviddias]

In my laptop, running new CID() 10,000 times took about 70 ~ 75 ms with the current code. After the change, the time was about 17 ~ 20 ms. The interesting thing was that the time from the current code sometimes went up ~100 ms once in a while.

Super cool 👏🏽👏🏽👏🏽👏🏽👏🏽 This is rad, I'm sure it will have a real perf improv in js-ipfs given that we use CID creation a lot.