valuable icon indicating copy to clipboard operation
valuable copied to clipboard

Implement valuable-json

Open taiki-e opened this issue 4 years ago • 5 comments

taiki-e avatar May 20 '21 12:05 taiki-e

Currently, this is almost the same as the JSON serializer example that I previously wrote. Hopefully, I can get the rest of the implementation work done over the weekend and into next week.

taiki-e avatar May 20 '21 14:05 taiki-e

Looks good to me :+1:

carllerche avatar May 20 '21 18:05 carllerche

This is now ready for review.

taiki-e avatar Jun 08 '21 12:06 taiki-e

Hey! :wave: I've got some simple benchmarks over in sval to make sure it stays in the same ballpark as other serialization libs and thought I'd pull in valuable to see where everything sits. When I first looked at it things were pretty even, but it looks like something might have regressed in valuable between 2665f14e9f3583f9d78ad1894c934d036d4fa492 and latest commit here (escaping was introduced in there, but shouldn't explain the whole jump, since all the serialization crates use basically the same escaping implementation).

The benchmark using valuable is the twitter_valuable one.

On 2665f14e9f3583f9d78ad1894c934d036d4fa492:

running 17 tests
test primitive_erased_serde         ... bench:          60 ns/iter (+/- 2)
test primitive_miniserde            ... bench:          62 ns/iter (+/- 2)
test primitive_serde                ... bench:          46 ns/iter (+/- 1)
test primitive_sval                 ... bench:          63 ns/iter (+/- 2)
test primitive_sval_noop            ... bench:           1 ns/iter (+/- 0)
test primitive_valuable             ... bench:          81 ns/iter (+/- 1)
test twitter_erased_serde           ... bench:     522,620 ns/iter (+/- 13,146)
test twitter_miniserde              ... bench:     777,410 ns/iter (+/- 56,092)
test twitter_serde                  ... bench:     352,435 ns/iter (+/- 5,395)
test twitter_serde_collect          ... bench:   2,383,070 ns/iter (+/- 124,506)
test twitter_serde_to_sval          ... bench:     551,045 ns/iter (+/- 19,296)
test twitter_serde_to_sval_to_serde ... bench:     701,745 ns/iter (+/- 29,351)
test twitter_sval                   ... bench:     530,075 ns/iter (+/- 19,094)
test twitter_sval_collect           ... bench:   2,611,290 ns/iter (+/- 80,522)
test twitter_sval_noop              ... bench:     182,645 ns/iter (+/- 34,668)
test twitter_sval_to_serde          ... bench:     696,730 ns/iter (+/- 121,546)
test twitter_valuable               ... bench:     490,240 ns/iter (+/- 5,443)

On ef976c01436e68cb81488014c79ec778febf62e7 (latest):

running 17 tests
test primitive_erased_serde         ... bench:          56 ns/iter (+/- 2)
test primitive_miniserde            ... bench:          62 ns/iter (+/- 2)
test primitive_serde                ... bench:          47 ns/iter (+/- 1)
test primitive_sval                 ... bench:          63 ns/iter (+/- 3)
test primitive_sval_noop            ... bench:           1 ns/iter (+/- 0)
test primitive_valuable             ... bench:          87 ns/iter (+/- 3)
test twitter_erased_serde           ... bench:     515,500 ns/iter (+/- 3,801)
test twitter_miniserde              ... bench:     761,630 ns/iter (+/- 27,143)
test twitter_serde                  ... bench:     344,995 ns/iter (+/- 2,094)
test twitter_serde_collect          ... bench:   2,349,262 ns/iter (+/- 129,222)
test twitter_serde_to_sval          ... bench:     555,985 ns/iter (+/- 38,844)
test twitter_serde_to_sval_to_serde ... bench:     701,045 ns/iter (+/- 45,682)
test twitter_sval                   ... bench:     533,130 ns/iter (+/- 9,658)
test twitter_sval_collect           ... bench:   2,620,105 ns/iter (+/- 139,792)
test twitter_sval_noop              ... bench:     180,693 ns/iter (+/- 10,653)
test twitter_sval_to_serde          ... bench:     689,275 ns/iter (+/- 17,616)
test twitter_valuable               ... bench:   1,807,990 ns/iter (+/- 36,908)

I've pushed my local changes with some valuable benches up here in case you wanted to have a poke. Don't mind all the other mess in there, I've been playing with other things which is why I was running these benches in the first place.

I haven't dug into the code for valuable-json much yet, I'm guessing there's some intermediate allocations or something going on? You might also be all over it, but thought I'd bring it up anyways 🙂🙏

KodrAus avatar Jun 24 '21 07:06 KodrAus

Thanks @KodrAus! That benchmark is very useful.


It seems that one of the problems is that the escape function is not inlined. Inlining the escape function would greatly improve performance.

Before (2374aa8073b013c09e0ce6398502c53ff24744fc):

test twitter_valuable   ... bench:   1,890,449 ns/iter (+/- 121,674)

After (07a320cb87693ead56754c32db82bc377cbd5360):

test twitter_valuable   ... bench:   1,010,623 ns/iter (+/- 198,776)

However, it's still much slower than 2665f14e9f3583f9d78ad1894c934d036d4fa492.

And, escaping seems to account for around 40% of the total. The following are the results when escaping is disabled (f7e335ce2c71afbe2c3a4eb420ffebc61540e36b).

test twitter_valuable   ... bench:     634,698 ns/iter (+/- 53,980)

Also, the following two changes will also improve performance:

  • use serde_json's escaping implementation (https://github.com/serde-rs/json/pull/89).
  • remove utf8 check
test twitter_valuable     ... bench:     812,378 ns/iter (+/- 29,584)

One of the rest of the problems is that the style options are checked frequently. Removing them will look like the following:

test twitter_erased_serde ... bench:     609,975 ns/iter (+/- 92,778)
test twitter_miniserde    ... bench:     988,489 ns/iter (+/- 62,401)
test twitter_serde        ... bench:     419,275 ns/iter (+/- 43,249)
test twitter_sval         ... bench:     663,869 ns/iter (+/- 32,564)
test twitter_valuable     ... bench:     685,965 ns/iter (+/- 38,047)

taiki-e avatar Jun 24 '21 13:06 taiki-e