-
Notifications
You must be signed in to change notification settings - Fork 171
Support canonical encoding by adding sortKeys option to encode() #64
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
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #64 +/- ## ========================================== - Coverage 96.9% 96.89% -0.01% ========================================== Files 15 15 Lines 905 903 -2 Branches 183 183 ========================================== - Hits 877 875 -2 Misses 28 28
Continue to review full report at Codecov.
|
FYI @sergeyzenchenko
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.
LGTM, but left one comment about sorting. Take a look at it
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.
Maybe we should have two separate branches? One with keys array and one without? it will be better from performance perspective. But maybe it's over optimization
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.
Maybe you are right because most of the cases do not set sortKeys.
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.
Unfortunately, Object.keys() version is always faster.
benchmark script:
import { encode, decode } from "../src"; // @ts-ignore import _ from "lodash"; const data: Record<string, number> = {}; for (let i = 0; i < 100; i++) { data["foo" + i] = i; } // warm up const encoded = encode(data); decode(encoded); // run console.time("encode sortKeys: false"); for (let i = 0; i < 100000; i++) { encode(data, { sortKeys: false }); } console.timeEnd("encode sortKeys: false"); console.time("encode sortKeys: true"); for (let i = 0; i < 100000; i++) { encode(data, { sortKeys: true }); } console.timeEnd("encode sortKeys: true");
result of for-in version:
$ npx ts-node benchmark/sandbox.ts
encode sortKeys: false: 2681.488ms
encode sortKeys: true: 2374.899ms
result of Object.keys() version (i.e. the same as this PR):
$ npx ts-node benchmark/sandbox.ts
encode sortKeys: false: 1336.151ms
encode sortKeys: true: 2343.010ms
I think for-in version uses less memory, but it's much slower.
So I don't change this PR.
sortKeys