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

check unexpected high bits for invalid characters #173

Merged
merged 3 commits into from
Feb 1, 2018
Merged

check unexpected high bits for invalid characters #173

merged 3 commits into from
Feb 1, 2018

Conversation

dcousens
Copy link
Contributor

@dcousens dcousens commented Nov 30, 2017

10% performance drop, but 200% better debuggability.

Hopefully this resolves that problem @indutny

@dcousens
Copy link
Contributor Author

dcousens commented Nov 30, 2017

passes tests now

'0000gg00',
'ffffggff',
'ffffggff',
'hexadecimal'
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add ., -, /. They all come before 0 in ASCII.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@indutny added, and it passes!

@indutny
Copy link
Owner

indutny commented Dec 2, 2017

Sorry for delay. I just benchmarked it, and don't see any significant performance improvement. Do you see it on your side?


Benchmarks can be run with:

SELF_ONLY=1 node benchmarks/index.js create

@dcousens
Copy link
Contributor Author

dcousens commented Dec 4, 2017

I measured an average 5-10% improvement... but [assumedly] the GC totally throws it off, and I often measure +/-20% swings for both depending on their order in the benchmark stack.

@fanatid
Copy link
Collaborator

fanatid commented Dec 5, 2017

node version: v9.2.0

master:

Seed: 82910582182c79d7d4d9592e15d53f0d
Benchmarking: create-10
bn.js#create-10 x 376,678 ops/sec ±1.55% (9 runs sampled)
------------------------
Fastest is bn.js#create-10
========================
Benchmarking: create-hex
bn.js#create-hex x 478,025 ops/sec ±1.02% (9 runs sampled)
------------------------
Fastest is bn.js#create-hex
========================

PR:

Seed: 82910582182c79d7d4d9592e15d53f0d
Benchmarking: create-10
bn.js#create-10 x 383,537 ops/sec ±0.90% (9 runs sampled)
------------------------
Fastest is bn.js#create-10
========================
Benchmarking: create-hex
bn.js#create-hex x 489,839 ops/sec ±0.37% (9 runs sampled)
------------------------
Fastest is bn.js#create-hex
========================

@dcousens
Copy link
Contributor Author

dcousens commented Dec 5, 2017

@fanatid how did you get consistent results :P

@fanatid
Copy link
Collaborator

fanatid commented Dec 5, 2017

Sorry, was run with different seed, numbers updated.

@dcousens
Copy link
Contributor Author

dcousens commented Feb 1, 2018

@indutny good to merge? If not, I'll simply add the test?

Copy link
Owner

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for delay.

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

Successfully merging this pull request may close these issues.

3 participants