Skip to content

Conversation

@gfx
Copy link
Member

@gfx gfx commented Jun 11, 2019

@gfx gfx requested a review from sergeyzenchenko June 11, 2019 14:29
@gfx gfx added the Feature label Jun 11, 2019
@codecov-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

Merging #64 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/Encoder.ts 97.78% <100%> (-0.02%) ⬇️
src/encode.ts 100% <100%> (ø) ⬆️

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 aed070f...3c0ee99. Read the comment docs.

@gfx
Copy link
Member Author

gfx commented Jun 12, 2019

FYI @sergeyzenchenko

Copy link
Collaborator

@sergeyzenchenko sergeyzenchenko left a 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

encodeMap(object: Record<string, unknown>, depth: number) {
const size = this.countObjectKeys(object);
const keys = Object.keys(object);
if (this.sortKeys) {
Copy link
Collaborator

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

Copy link
Member Author

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.

Copy link
Member Author

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.

@gfx gfx merged commit dc24384 into master Jun 12, 2019
@gfx gfx deleted the sort_keys branch June 12, 2019 13:52
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.

4 participants