Skip to content

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.

Sign up for GitHub

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

Merged
gfx merged 2 commits into from
Jun 12, 2019
Merged

Support canonical encoding by adding sortKeys option to encode() #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d54ad1
add sortKeys option to encode()
gfx Jun 11, 2019
3c0ee99
remove unused utility
gfx Jun 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ npm install @msgpack/msgpack

It encodes `data` and returns a byte array as `Uint8Array`.

#### EncodeOptions

Name|Type|Default
----|----|----
extensionCodec | ExtensionCodec | `ExtensinCodec.defaultCodec`
maxDepth | number | `100`
initialBufferSize | number | `2048`
sortKeys | boolean | false

### `decode(buffer: ArrayLike<number>, options?: DecodeOptions): unknown`

It decodes `buffer` encoded as MessagePack, and returns a decoded object as `uknown`.
Expand Down
27 changes: 11 additions & 16 deletions src/Encoder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export class Encoder {
readonly extensionCodec = ExtensionCodec.defaultCodec,
readonly maxDepth = DEFAULT_MAX_DEPTH,
readonly initialBufferSize = DEFAULT_INITIAL_BUFFER_SIZE,
readonly sortKeys = false,
) {}

encode(object: unknown, depth: number): void {
Expand Down Expand Up @@ -223,18 +224,12 @@ export class Encoder {
}
}

countObjectKeys(object: Record<string, unknown>): number {
let count = 0;
for (const key in object) {
if (Object.prototype.hasOwnProperty.call(object, key)) {
count++;
}
}
return count;
}

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

@sergeyzenchenko sergeyzenchenko Jun 12, 2019

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

@gfx gfx Jun 12, 2019

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

@gfx gfx Jun 12, 2019

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.

keys.sort();
}
const size = keys.length;
if (size < 16) {
// fixmap
this.writeU8(0x80 + size);
Expand All @@ -249,11 +244,11 @@ export class Encoder {
} else {
throw new Error(`Too large map object: ${size}`);
}
for (const key in object) {
if (Object.prototype.hasOwnProperty.call(object, key)) {
this.encodeString(key);
this.encode(object[key], depth + 1);
}

for (let i = 0; i < size; i++) {
const key = keys[i];
this.encodeString(key);
this.encode(object[key], depth + 1);
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/encode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ export type EncodeOptions = Partial<
extensionCodec: ExtensionCodecType;
maxDepth: number;
initialBufferSize: number;
sortKeys: boolean;
}>
>;

const defaultEncodeOptions = {};

export function encode(value: unknown, options: EncodeOptions = defaultEncodeOptions): Uint8Array {
const encoder = new Encoder(options.extensionCodec, options.maxDepth, options.initialBufferSize);
const encoder = new Encoder(options.extensionCodec, options.maxDepth, options.initialBufferSize, options.sortKeys);
encoder.encode(value, 1);
return encoder.getUint8Array();
}
13 changes: 13 additions & 0 deletions test/encode-options.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import assert from "assert";
import { encode } from "@msgpack/msgpack";

describe("encode options", () => {
context("sortKeys", () => {
it("canonicalize encoded binaries", () => {
assert.deepStrictEqual(
encode({ a: 1, b: 2 }, { sortKeys: true }),
encode({ b: 2, a: 1 }, { sortKeys: true }),
);
});
});
});