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

Use jsbi for internal 64bit integer calculations #980

Merged
merged 5 commits into from Oct 16, 2019
Merged

Conversation

arthurschreiber
Copy link
Collaborator

This switches the code that is used for doing internal 64bit number calculations from our own custom code over to jsbi. jsbi is based on the BigInt type available in Node.js 12 and newer, but works in all Node.js versions that we want to be supported by tedious.

Context

Some parts of the TDS protocal use 64-bit integers. Examples for this are the rowcount in the DONE token, or everything related to the bigint data type.

The code that was used for this was adapted from different other libraries (e.g. node-int64), but was never optimized for speed nor correctness (see #973). By replacing this code with jsbi, we should be able to improve performance, reduce the code we need to maintain, and also ensure correctness.

Note: This only changes internals. All external code reading / writing to bigint columns will be unaffected (but should be much faster than before).

@arthurschreiber arthurschreiber force-pushed the arthur/jsbi branch 2 times, most recently from cf2a759 to e666077 Compare October 2, 2019 21:24
parser.readUInt32LE(next);
} else {
parser.readBigUInt64LE((rowCount) => {
next(JSBI.toNumber(rowCount));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If more than Number.MAX_SAFE_INTEGER (9007199254740991) rows are transferred, this conversion might result in an incorrect rowcount. I'm not sure if that's something we have to care about - this limit seems hardly reachable. 🤷‍♂

@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #980 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
- Coverage   83.34%   83.22%   -0.12%     
==========================================
  Files          87       86       -1     
  Lines        4274     4250      -24     
  Branches      373      366       -7     
==========================================
- Hits         3562     3537      -25     
- Misses        594      597       +3     
+ Partials      118      116       -2
Impacted Files Coverage Δ
src/value-parser.ts 81.03% <100%> (-0.07%) ⬇️
src/tracking-buffer/writable-tracking-buffer.ts 92.61% <100%> (+0.95%) ⬆️
src/ntlm-payload.ts 94.54% <100%> (+1.38%) ⬆️
src/token/done-token-parser.ts 100% <100%> (ø) ⬆️
src/token/stream-parser.ts 74.54% <100%> (-1.1%) ⬇️
src/data-types/bigint.ts 90% <0%> (+5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d83256f...b969541. Read the comment docs.

@arthurschreiber arthurschreiber merged commit f2336c2 into master Oct 16, 2019
@arthurschreiber arthurschreiber deleted the arthur/jsbi branch October 16, 2019 05:57
@arthurschreiber
Copy link
Collaborator Author

🎉 This PR is included in version 6.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants