This repository has been archived by the owner on Sep 3, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 39
fix: #39 - Improve constructor performance #45
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b2b5d2f
fix: #39 - Improve constructor performance
realharry 7eaaa02
Added dev dependency
realharry fd65693
Updated dev dependencies
realharry 4d423a7
Updated based on the feedback
realharry bdf3404
Fixed checkin error
realharry c2cb1c7
Updated per review/feedback
realharry 0f11f0c
checkCIDComponents() method now does not throw error.
realharry File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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' | ||
} | ||
|
||
if (!(other.version === 0 || other.version === 1)) { | ||
return 'Invalid version, must be a number equal to 1 or 0' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. plus this one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
if (typeof other.codec !== 'string') { | ||
return 'codec must be string' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. plus this one There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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() | ||
})) | ||
}) | ||
}) | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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') | ||
}) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.