well formed unicode string (#147) by srmarjani · Pull Request #151 · fastify/fast-json-stringify

srmarjani · 2019-03-23T09:36:47Z

I changed $asStringSmall function so It can detect surrogate character.
But in surrogate.test.js if you uncomment line 50 and 65 , test will fail.
I am using node 11.12.0.
Is JSON.Stringify() is up to date or what @mathiasbynens has said in issue 147 is in test phase?

mcollina

How much does this impact our benchmarks?

Overall this sounds correct, even if we don’t have the fixed V8 yet.

srmarjani · 2019-03-23T10:04:04Z

In $asStringSmall there is one for that loops over input string. I have put surrogate detector condition in this loop. So I think it will has no impact on benchmarks.
But I have not tested it.

mcollina · 2019-03-23T10:33:46Z

Just run https://github.com/fastify/fast-json-stringify/blob/master/bench.js before and after your change.

srmarjani · 2019-03-23T13:57:58Z

befor:
FJS creation x 2,556 ops/sec ±8.41% (68 runs sampled)
JSON.stringify array x 1,323 ops/sec ±7.12% (65 runs sampled)
fast-json-stringify array x 2,723 ops/sec ±6.71% (67 runs sampled)
fast-json-stringify-uglified array x 1,729 ops/sec ±14.74% (56 runs sampled)
JSON.stringify long string x 7,548 ops/sec ±5.59% (68 runs sampled)
fast-json-stringify long string x 7,639 ops/sec ±5.20% (71 runs sampled)
fast-json-stringify-uglified long string x 5,762 ops/sec ±9.27% (54 runs sampled)
JSON.stringify short string x 2,427,132 ops/sec ±6.54% (63 runs sampled)
fast-json-stringify short string x 11,023,374 ops/sec ±4.68% (72 runs sampled)
fast-json-stringify-uglified short string x 10,934,208 ops/sec ±4.34% (70 runs sampled)
JSON.stringify obj x 682,282 ops/sec ±9.87% (61 runs sampled)
fast-json-stringify obj x 2,365,212 ops/sec ±7.54% (68 runs sampled)
fast-json-stringify-uglified obj x 2,571,663 ops/sec ±7.17% (69 runs sampled)

srmarjani · 2019-03-23T13:58:28Z

after:
FJS creation x 613 ops/sec ±14.72% (44 runs sampled)
JSON.stringify array x 455 ops/sec ±14.29% (40 runs sampled)
fast-json-stringify array x 1,506 ops/sec ±6.85% (55 runs sampled)
fast-json-stringify-uglified array x 1,489 ops/sec ±3.84% (65 runs sampled)
JSON.stringify long string x 4,807 ops/sec ±3.92% (64 runs sampled)
fast-json-stringify long string x 5,278 ops/sec ±5.35% (66 runs sampled)
fast-json-stringify-uglified long string x 4,972 ops/sec ±3.71% (63 runs sampled)
JSON.stringify short string x 1,555,898 ops/sec ±4.50% (64 runs sampled)
fast-json-stringify short string x 9,776,310 ops/sec ±5.19% (67 runs sampled)
fast-json-stringify-uglified short string x 9,768,031 ops/sec ±6.04% (64 runs sampled)
JSON.stringify obj x 461,803 ops/sec ±4.94% (60 runs sampled)
fast-json-stringify obj x 1,703,510 ops/sec ±6.36% (52 runs sampled)
fast-json-stringify-uglified obj x 1,420,784 ops/sec ±3.57% (59 runs sampled)

mcollina · 2019-03-23T15:09:53Z

You were likely running something else on your machines, as in the “after” all JSON.stringify benchmarks went down ad well. Can you rerun?

mathiasbynens · 2019-03-23T15:24:21Z

index.js

 // magically escape strings for json
 // relying on their charCodeAt
 // everything below 32 needs JSON.stringify()
+// every string that contain surrogate needs JSON.stringify()


This is not accurate. Every lone surrogate needs escaping. Surrogates that appear in valid surrogate pairs must not be escaped.

I know @mathiasbynens
I read your Implementation In c. But I think implementing it in JavaScript would be heavy.
The approach of @mcollina is an heuristic approach not deterministic.
Using of surrogate in keys of object in js is not common.
So I decided to use standard stringify when we encounter such character

I think we should clarify that comment and write down why we use that heuristics: doing the full algorithm here would be too expensive. However the penalty to try this for lower length strings and falling back to JSON.stringify is ok.

FWIW, I think this approach makes total sense. Lone surrogates are an edge case after all.

test/surrogate.test.js

srmarjani · 2019-03-23T17:57:48Z

Hi @mcollina
You have time to run benchmark?
I get different result in each run.
Maybe there is some problem in my laptop :))

mcollina · 2019-03-25T10:15:55Z

test/surrogate.test.js

+  const validate = validator(schema)
+  const stringify = build(schema)
+  const output = stringify('\uDF06\uD834')
+  // t.equal(output, '"\\udf06\\ud834"')


It would be great to use a mirror test for this.

t.equal(output, JSON.stringify('\uDF06\uD834'))

What do you mean by mirror test?(Excuse me if this question is simple :)) )

What I put above. Basically you have a target system A. You want to verify that B calls A and returns that output. You call B() and then A() and you compare results. In this way any change of output in A() will be automatically reflected in B() output.

Thanks. I got it.

mcollina · 2019-03-25T10:50:29Z

It seems we are losing a bit:

~/repositories/fast-json-stringify (147-well-formed-unicode-string)
$ node bench.js
FJS creation x 8,953 ops/sec ±0.47% (87 runs sampled)
JSON.stringify array x 5,165 ops/sec ±0.23% (97 runs sampled)
fast-json-stringify array x 8,315 ops/sec ±0.48% (94 runs sampled)
fast-json-stringify-uglified array x 8,549 ops/sec ±0.27% (99 runs sampled)
JSON.stringify long string x 13,099 ops/sec ±0.13% (99 runs sampled)
fast-json-stringify long string x 13,104 ops/sec ±0.11% (98 runs sampled)
fast-json-stringify-uglified long string x 13,068 ops/sec ±0.22% (96 runs sampled)
JSON.stringify short string x 6,342,822 ops/sec ±0.08% (95 runs sampled)
fast-json-stringify short string x 40,577,317 ops/sec ±1.25% (92 runs sampled)
fast-json-stringify-uglified short string x 43,137,917 ops/sec ±0.74% (90 runs sampled)
JSON.stringify obj x 2,509,518 ops/sec ±0.53% (97 runs sampled)
fast-json-stringify obj x 8,766,504 ops/sec ±0.34% (97 runs sampled)
fast-json-stringify-uglified obj x 8,847,147 ops/sec ±0.26% (96 runs sampled)
~/repositories/fast-json-stringify (147-well-formed-unicode-string)

~/repositories/fast-json-stringify (master)
$ node bench.js
FJS creation x 8,868 ops/sec ±0.40% (92 runs sampled)
JSON.stringify array x 5,169 ops/sec ±0.23% (98 runs sampled)
fast-json-stringify array x 8,414 ops/sec ±0.66% (94 runs sampled)
fast-json-stringify-uglified array x 8,628 ops/sec ±0.36% (95 runs sampled)
JSON.stringify long string x 13,133 ops/sec ±0.06% (99 runs sampled)
fast-json-stringify long string x 13,111 ops/sec ±0.15% (99 runs sampled)
fast-json-stringify-uglified long string x 13,121 ops/sec ±0.08% (98 runs sampled)
JSON.stringify short string x 6,286,166 ops/sec ±0.11% (100 runs sampled)
fast-json-stringify short string x 44,704,671 ops/sec ±0.99% (93 runs sampled)
fast-json-stringify-uglified short string x 39,767,374 ops/sec ±1.41% (85 runs sampled)
JSON.stringify obj x 2,496,478 ops/sec ±0.48% (99 runs sampled)
fast-json-stringify obj x 9,197,710 ops/sec ±0.24% (94 runs sampled)
fast-json-stringify-uglified obj x 9,005,966 ops/sec ±0.30% (97 runs sampled)


I think this is the best we can get, overall +1 once the test is done.

srmarjani added 2 commits March 23, 2019 13:55

surrogate is detected and defered to JSON.stringify()

c3cc694

remove console log from test

8e22f1b

mcollina reviewed Mar 23, 2019

View reviewed changes

mathiasbynens reviewed Mar 23, 2019

View reviewed changes

test/surrogate.test.js Outdated Show resolved Hide resolved

mcollina reviewed Mar 25, 2019

View reviewed changes

for lone and unpair surrogate mirror test used

60143be

srmarjani changed the title ~~WIP-147 well formed unicode string (Please Don't Merge)~~ well formed unicode string (#147) Mar 25, 2019

mcollina merged commit eb5e9b1 into fastify:master Mar 27, 2019

yakkomajuri mentioned this pull request Apr 8, 2022

fix(ingestion): escape surrogate characters when producing to kafka PostHog/posthog#9368

Merged

Uh oh!

Conversation

srmarjani commented Mar 23, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

srmarjani commented Mar 23, 2019

Uh oh!

mcollina commented Mar 23, 2019

Uh oh!

srmarjani commented Mar 23, 2019

Uh oh!

srmarjani commented Mar 23, 2019

Uh oh!

mcollina commented Mar 23, 2019

Uh oh!

mathiasbynens Mar 23, 2019

Choose a reason for hiding this comment

Uh oh!

srmarjani Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

mathiasbynens Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srmarjani commented Mar 23, 2019

Uh oh!

mcollina Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

srmarjani Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

srmarjani Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

srmarjani Mar 23, 2019 •

edited

Loading