Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance #100

Closed
wants to merge 0 commits into from
Closed

Improve performance #100

wants to merge 0 commits into from

Conversation

magicode
Copy link
Contributor

No description provided.

index.js Outdated
@@ -5,9 +5,9 @@ const oxmlContentTypes = toBytes('[Content_Types].xml');
const oxmlRels = toBytes('_rels/.rels');

module.exports = input => {
const buf = new Uint8Array(input);
Copy link
Owner

Choose a reason for hiding this comment

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

This is intentional, for browser support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this make new buffer it It takes performance.
Why an array of numbers is not good for checking?

@sindresorhus
Copy link
Owner

I appreciate the PR, but it's missing a benchmark proving the numbers you claim :)

@magicode
Copy link
Contributor Author

magicode commented Sep 4, 2017

try this

const fs = require('fs');
const filetype = require('.');

var file = fs.readFileSync(__dirname + "/fixture/fixture.png");

var time = 3000;
var starttime = Date.now();
var endtime = starttime + time;
var count = 0;
for(;Date.now()<endtime;count++){
    filetype(file);
}

console.log("%d ops/s" ,~~(count/((Date.now()-starttime)/1000)));

@sindresorhus
Copy link
Owner

@mifi Any thoughts on this?

index.js Outdated

if (!(buf && buf.length > 1)) {
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

It's a bit unclear why u compare with -1, why not use buf[1] !== undefined ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks that it is also a number

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Why is that needed? also '' > -1 is true which is obviously not a number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a check that on the way.
It's not important

@mifi
Copy link
Sponsor Contributor

mifi commented Oct 8, 2017

I dunno if some of our users are sending in an array, but if they do, some file type checks will fail when removing the wrapping, like those using buf.subarray(), because array does not have this method.
Our docs however say clearly that we input should be Buffer or Uint8array. Still if we do this change i think we should bump major version.

Also, is it the removal of the wrapping or the length check that boosts performance?

@magicode
Copy link
Contributor Author

magicode commented Oct 8, 2017

What's important for performance.
It does not wrap if it Buffer or Uint8array.

@mifi
Copy link
Sponsor Contributor

mifi commented Oct 8, 2017

Ok, I think then you can revert the buf[1] > -1 change, if it is not important for performance.
Also, don't assign const buf = input;, instead rename argument input to buf.
Then I think this can be merged if @sindresorhus agrees.

@sindresorhus sindresorhus changed the title up 4x faster Improve performance Oct 10, 2017
@sindresorhus
Copy link
Owner

Ok, sure, when @mifi's feedback is addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants