Skip to content
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

Implement valuable-json #40

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Implement valuable-json #40

wants to merge 22 commits into from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented May 20, 2021

No description provided.

@taiki-e
Copy link
Member Author

taiki-e commented May 20, 2021

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.

@carllerche
Copy link
Member

Looks good to me 👍

@taiki-e taiki-e marked this pull request as ready for review June 7, 2021 15:53
@taiki-e
Copy link
Member Author

taiki-e commented Jun 8, 2021

This is now ready for review.

@taiki-e taiki-e requested a review from carllerche June 8, 2021 12:45
@KodrAus
Copy link

KodrAus commented Jun 24, 2021

Hey! 👋 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 🙂🙏

@taiki-e
Copy link
Member Author

taiki-e commented Jun 24, 2021

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 (2374aa8):

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

After (07a320c):

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

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

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

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

Also, the following two changes will also improve performance:

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants