Skip to content

check unexpected high bits for invalid characters#173

Merged
dcousens merged 3 commits intomasterfrom
chk
Feb 1, 2018
Merged

check unexpected high bits for invalid characters#173
dcousens merged 3 commits intomasterfrom
chk

Conversation

@dcousens
Copy link
Copy Markdown
Contributor

@dcousens dcousens commented Nov 30, 2017 •
edited
Loading

10% performance drop, but 200% better debuggability.

Hopefully this resolves that problem @indutny

@dcousens
Copy link
Copy Markdown
Contributor Author

dcousens commented Nov 30, 2017 •
edited
Loading

passes tests now

dcousens referenced this pull request Nov 30, 2017
@indutny
lib: better validation of string input
e924509 
Fix: #151
indutny
indutny reviewed Nov 30, 2017
View reviewed changes
test/constructor-test.js
'0000gg00',
'ffffggff',
'ffffggff',
'hexadecimal'
Copy link
Copy Markdown
Owner

@indutny indutny Nov 30, 2017

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
Copy Markdown
Contributor Author

@dcousens dcousens Nov 30, 2017

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
Copy Markdown
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
Copy Markdown
Contributor Author

dcousens commented Dec 4, 2017 •
edited
Loading

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
Copy Markdown
Collaborator

fanatid commented Dec 5, 2017 •
edited
Loading

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
Copy Markdown
Contributor Author

dcousens commented Dec 5, 2017

@fanatid how did you get consistent results :P

@fanatid
Copy link
Copy Markdown
Collaborator

fanatid commented Dec 5, 2017

Sorry, was run with different seed, numbers updated.

@dcousens
Copy link
Copy Markdown
Contributor Author

dcousens commented Feb 1, 2018 •
edited
Loading

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

indutny
indutny approved these changes Feb 1, 2018
View reviewed changes
Copy link
Copy Markdown
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.

@dcousens dcousens merged commit 98ac84e into master Feb 1, 2018
@dcousens dcousens deleted the chk branch February 1, 2018 22:15
@exoego exoego mentioned this pull request Sep 11, 2019
Closed
@fanatid fanatid mentioned this pull request Dec 23, 2019
Closed
@meck93 meck93 mentioned this pull request Jan 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@indutny indutny indutny approved these changes

Assignees

No one assigned

Labels

feature / refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dcousens @indutny @fanatid