Skip to content
This repository has been archived by the owner on Sep 3, 2021. It is now read-only.

fix: #39 - Improve constructor performance #45

Merged
merged 7 commits into from
Mar 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
"aegir": "^12.0.8",
"chai": "^4.1.2",
"dirty-chai": "^2.0.1",
"multihashing-async": "~0.4.6",
"multihashing": "^0.3.2",
"multihashing-async": "^0.4.8",
"pre-commit": "^1.2.2"
},
"engines": {
Expand Down
43 changes: 43 additions & 0 deletions src/cid-util.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict'

const mh = require('multihashes')

var CIDUtil = {
/**
* Test if the given input is a valid CID object.
* Returns an error message if it is not.
* Returns undefined if it is a valid CID.
*
* @param {any} other
* @returns {string}
*/
checkCIDComponents: function (other) {
if (other == null) {
return 'null values are not valid CIDs'
Copy link
Member

Choose a reason for hiding this comment

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

This should also be a throw Error

Copy link
Member

Choose a reason for hiding this comment

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

"not anymore, since throwing errors makes everything slower.

}

if (!(other.version === 0 || other.version === 1)) {
return 'Invalid version, must be a number equal to 1 or 0'
Copy link
Member

Choose a reason for hiding this comment

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

plus this one

Copy link
Member

Choose a reason for hiding this comment

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

}

if (typeof other.codec !== 'string') {
return 'codec must be string'
Copy link
Member

Choose a reason for hiding this comment

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

plus this one

Copy link
Member

Choose a reason for hiding this comment

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

}

if (!Buffer.isBuffer(other.multihash)) {
return 'multihash must be a Buffer'
}

try {
mh.validate(other.multihash)
} catch (err) {
let errorMsg = err.message
if (!errorMsg) { // Just in case mh.validate() throws an error with empty error message
errorMsg = 'Multihash validation failed'
}
return errorMsg
}
}
}

module.exports = CIDUtil
28 changes: 5 additions & 23 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const multicodec = require('multicodec')
const codecs = require('multicodec/src/base-table')
const codecVarints = require('multicodec/src/varint-table')
const multihash = require('multihashes')
const CIDUtil = require('./cid-util')

/**
* @typedef {Object} SerializedCID
Expand Down Expand Up @@ -218,13 +219,7 @@ class CID {
* @returns {bool}
*/
static isCID (other) {
try {
CID.validateCID(other)
} catch (err) {
return false
}

return true
return !(CIDUtil.checkCIDComponents(other))
}

/**
Expand All @@ -235,23 +230,10 @@ class CID {
* @returns {void}
*/
static validateCID (other) {
if (other == null) {
throw new Error('null values are not valid CIDs')
let errorMsg = CIDUtil.checkCIDComponents(other)
if (errorMsg) {
throw new Error(errorMsg)
}

if (!(other.version === 0 || other.version === 1)) {
throw new Error('Invalid version, must be a number equal to 1 or 0')
}

if (typeof other.codec !== 'string') {
throw new Error('codec must be string')
}

if (!Buffer.isBuffer(other.multihash)) {
throw new Error('multihash must be a Buffer')
}

mh.validate(other.multihash)
}
}

Expand Down
75 changes: 75 additions & 0 deletions test/cid-util.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/* eslint-env mocha */
/* eslint max-nested-callbacks: ["error", 8] */
'use strict'

const chai = require('chai')
const dirtyChai = require('dirty-chai')
const expect = chai.expect
chai.use(dirtyChai)
const multihashing = require('multihashing-async')
const CID = require('../src')
const CIDUtil = require('../src/cid-util')

describe('CIDUtil', () => {
let hash

before((done) => {
multihashing(Buffer.from('abc'), 'sha2-256', (err, d) => {
if (err) {
return done(err)
}
hash = d
done()
})
})

describe('checkCIDComponents()', () => {
describe('returns undefined on valid CID', () => {
it('create from B58Str multihash', () => {
expect(() => {
const mhStr = 'QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n'
const cid = new CID(mhStr)
const errMsg = CIDUtil.checkCIDComponents(cid)
expect(errMsg).to.not.exist()
}).to.not.throw()
})
it('create by parts', () => {
expect(() => {
const cid = new CID(1, 'dag-cbor', hash)
const errMsg = CIDUtil.checkCIDComponents(cid)
expect(errMsg).to.not.exist()
}).to.not.throw()
})
})

describe('returns non-null error message on invalid inputs', () => {
const invalid = [
'hello world',
'QmaozNR7DZHQK1ZcU9p7QdrshMvXqWK6gpu5rmrkPdT3L',
Buffer.from('hello world'),
Buffer.from('QmaozNR7DZHQK1ZcU9p7QdrshMvXqWK6gpu5rmrkPdT')
]

invalid.forEach((i) => it(`new CID(${Buffer.isBuffer(i) ? 'buffer' : 'string'}<${i.toString()}>)`, () => {
expect(() => {
const errMsg = CIDUtil.checkCIDComponents(i)
expect(errMsg).to.exist()
}).to.not.throw()
}))

invalid.forEach((i) => it(`new CID(0, 'dag-pb', ${Buffer.isBuffer(i) ? 'buffer' : 'string'}<${i.toString()}>)`, () => {
expect(() => {
const errMsg = CIDUtil.checkCIDComponents(0, 'dag-pb', i)
expect(errMsg).to.exist()
}).to.not.throw()
}))

invalid.forEach((i) => it(`new CID(1, 'dag-pb', ${Buffer.isBuffer(i) ? 'buffer' : 'string'}<${i.toString()}>)`, () => {
expect(() => {
const errMsg = CIDUtil.checkCIDComponents(1, 'dag-pb', i)
expect(errMsg).to.exist()
}).to.not.throw()
}))
})
})
})
52 changes: 52 additions & 0 deletions test/profiling/cidperf-x.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict'

const multihashing = require('multihashing')
// [1] Original/existing implementation.
// const CID = require('cids')
// [2] New/proposed implementation.
const CID = require('../../src')

// Used to delay the testing for a few seconds.
function sleep (ms) {
return new Promise(resolve => setTimeout(resolve, ms))
}

// Simple test class.
// The purpose of this class is
// to simply test the CID ctor (and, primarily, CID.isCID() method).
class CIDPerfX {
constructor () {
this.version = 1
this.codec = 'dag-pb'
this.mh = multihashing(Buffer.from('oh, hey!'), 'sha2-256')
}

// i: Running-counter.
// print: If true, it'll print/dump the CID data.
run (i, print) {
const cid = new CID(this.version, this.codec, this.mh)
if (print === true) {
console.log('i=' + i, cid)
}
}
}

// /////////////////////////
// Main test routine.
// Note: You will need to run the test separately
// for each "alternative" impls and compare their results.
// /////////////////////////

const reps = 10000
const cidPerf = new CIDPerfX()

console.log(`Test: Will run "new CID()" ${reps} times.`)
// We just give ~1 second for the JS engine to start and 'rest', etc.
// before starting new tests.
sleep(1000).then(() => {
console.log(`Starting a test...`)
console.time('run'); [...Array(reps).keys()].map(i => {
cidPerf.run(i)
})
console.timeEnd('run')
})