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

0.2 work #71

Merged
merged 34 commits into from Dec 1, 2019
Merged

0.2 work #71

merged 34 commits into from Dec 1, 2019

Conversation

@Licenser
Copy link
Member

Licenser commented Oct 23, 2019

This is a work branch for the 0.2 release where we can introduce breaking changes for things we do not like in 0.1.

So far:

  • Add U64 type
  • Remove deprecated functions
  • Box objects to reduce enum size
  • Add object and array access to value trait for convenience
  • Arch alignment
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #71 into master will decrease coverage by 4.63%.
The diff coverage is 70.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #71      +/-   ##
=========================================
- Coverage   82.23%   77.6%   -4.64%     
=========================================
  Files          34      36       +2     
  Lines        4307    4376      +69     
=========================================
- Hits         3542    3396     -146     
- Misses        765     980     +215
Flag Coverage Δ
#avx2 77.6% <70.42%> (+0.39%) ⬆️
#avx2KnownKey 77.6% <70.42%> (?)
#e 100% <ø> (?)
#sseKnownKey 100% <ø> (?)
#ssl ?
Impacted Files Coverage Δ
src/charutils.rs 100% <ø> (ø) ⬆️
tests/jsonchecker.rs 100% <ø> (ø) ⬆️
src/sse42/generator.rs 0% <ø> (-86.21%) ⬇️
src/avx2/generator.rs 100% <ø> (+10%) ⬆️
src/sse42/deser.rs 0% <0%> (-79.79%) ⬇️
src/sse42/utf8check.rs 0% <0%> (-100%) ⬇️
src/sse42/stage1.rs 0% <0%> (-94.57%) ⬇️
src/serde/value/borrowed/se.rs 0% <0%> (ø) ⬆️
src/avx2/utf8check.rs 100% <100%> (ø) ⬆️
src/utf8check.rs 100% <100%> (ø)
... and 30 more

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 e1f67cb...8eb9b11. Read the comment docs.

@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Oct 23, 2019

@sunnygleason anything you can think of API wise we should change?

@sunnygleason

This comment has been minimized.

Copy link
Member

sunnygleason commented Oct 23, 2019

@Licenser great question - would it be better to do the generic/template code work as part of this new branch, or better to get it into 0.1? I've been meaning to finish that work up sometime very soon...

@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Oct 23, 2019

Either works for me. We can change everything other then the outside facing API as much as we want :D.

The 0.2 branch so far changes 2 things: it removes some functions that were bad and marked as deprecated in 0.1 already and adds the U64 as a natively supported type.

Since users will never notice how the parsing code works under the hood it's not constrained by the same restrictions :D.

Even adding 128 bit support (since we probably would want to feature flag that for perf reasons?) would not change the API just add to it (which reminds me, no harm to add as_u128 and as_i128 to the value trait :D Seems we did that already!

@sunnygleason

This comment has been minimized.

Copy link
Member

sunnygleason commented Oct 23, 2019

how are we doing on benchmarks lately? I wonder, are there API changes or alternate entry points we could consider to increase parsing speed relative to native simdjson?

@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Oct 23, 2019

We don't have many benchmarks when it comes to the API, and that is a interesting topic since it is where simd-json.rs deviates a lot from simdjson.

There are 4 parts of the 'lifetime' of data:

  1. decoding / parsing
  2. reading / evaluating
  3. editing / mutating
  4. encoding / rendering

upstream simdjson is extremely good at 1) but makes 2) harder and probably slower and 3) nearly impossible. 4) I suspect will be faster as well since mutations do not happen.

A part of the speedup in 1) is by sacrificing 2) and 3) - instead of creating a DOM (like we do) it uses a tape, which is hands down faster - no allocation, no calculation of hashes, a unfragmented memory area.

However when reading that sacrifices a lot of convenience, you can't "take the 4th element of an array" or "get the key foo from a object" all let alone do so in a nested way. That all has to be hand rolled, for objects every key/value pair has to be checked until the right key is found, for an array each element has to be passed to find the nth one.

Mutation (3)) with that model is simply not possible without first creating some kind of mutable structure out of it (which either means parsing into an object (which we have from serde support) or creating a DOM (the same way we do on parsing.

simdjson, as a low level library lets you pick your battles and if for example you only ever read, is significantly faster. simd-json.rs is more opinionated towards end to end workflow and convenience. That said I think on with parsing to objects we are not significantly slower

Licenser added 14 commits Oct 30, 2019
* Box objects
This reverts commit f40a712.
No measurable performance benefit
This reverts commit b5eb02f.
@Licenser Licenser added this to To Do in 0.2 Nov 4, 2019
* Handle more parsing during stage2
* Move all parsing into stage2
* Remove conditionals in DOM generation
* Separate out static values
* Expose tape parsing
* Additional tests
@sunnygleason

This comment has been minimized.

Copy link
Member

sunnygleason commented Nov 5, 2019

@Licenser wow, this is an enormous set of changes. Is there anything we can do to merge them in stages, or should I just swallow the big pill 💊 and review all at once?

@sunnygleason

This comment has been minimized.

Copy link
Member

sunnygleason commented Nov 5, 2019

ok only about 800 lines left to review! will come back to it soon. this is looking nice, great work! 🍻

@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Nov 5, 2019

you can look at the topic based PR's:

I create a pr towards the 0.2 branch for every change, just merge them every few days so they don't block as they're all dependant on each other.

@sunnygleason

This comment has been minimized.

Copy link
Member

sunnygleason commented Nov 5, 2019

gagh! answering a PR with more PR's, run away! 🏃 💨

@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Nov 6, 2019

The only other things I was wondering about were:

  • would it be useful to specify what version of simdjson upstream this is tracking?

Yes! But we'd need to figure out what it is 😂

  • arch alignments to template-ize code, would that still be useful? (while we're changing everything 😝 )

Yes! either towards 0.1 or 0.2 I'd love if we'd get there! But as always, provided it's not a perf hit - sometimes generic code is not as fast as specific code, but we got our benchmarks and perf tests to decide on that ;)

In any case, this looks really nice, the benchmark results sound outstanding!

@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Nov 11, 2019

@sunnygleason lets do a quick planning on the 0.2 release, I think getting the "this is following simdjson vXXX in is a good goal to have. but we'd need to pick a version and then go over it to see how we track. Would you want to get the arch alignment changes into 0.2 as well? They kind of fit to that theme. Is there anything I'm missing for 0.2 we'd want

* feat: improve arch-specific code similarity
* fix: function signature, remove ParseStringHelper
* fix: pub(crate) on check_utf8_bytes
@sunnygleason

This comment has been minimized.

Copy link
Member

sunnygleason commented Nov 19, 2019

@Licenser I like that Value work you're doing. I'm doing an investigation of turning SimdInput into a struct/trait with associated functions to reduce/standardize code in stage1. Should have something (hopefully interesting) in the next couple days. Warm up the benchmarks! 😄 🍻

@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Nov 19, 2019

Sweet!

Licenser and others added 5 commits Nov 20, 2019
* Split traits into read, mutate and build
* Add generic deserialise
* Add some tests for generalised deserialiser
@Licenser Licenser marked this pull request as ready for review Nov 26, 2019
@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Nov 26, 2019

I'm good with merging this, @sunnygleason what do you think?

@sunnygleason

This comment has been minimized.

Copy link
Member

sunnygleason commented Nov 26, 2019

@Licenser yes, I think we've got it! If you think it could wait one more day, I was thinking of doing the utf8check and "upstream tracking version" updates early tomorrow morning (ET)... (that's internal & non-blocking though)

@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Nov 26, 2019

👍 absolutely, as long as there is stuff to be done we can wait :) put a not down when you're ready to go .- I want to avoid the everyone is waiting for someone else problem ;) so as soon as you're ready, I'm ready.

* feat: improve utf8 modularity w/ Utf8Check trait
* feat: align to upstream simdjson-0.2.1
* TODO: align stringparse and numberparse
@sunnygleason

This comment has been minimized.

Copy link
Member

sunnygleason commented Dec 1, 2019

@Licenser I think that's all the tricks I have for now, release anytime! 😁

Licenser added 3 commits Dec 1, 2019
@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Dec 1, 2019

Perf:

image

master

======= simd_json ======== parse|stringify ===== parse|stringify ====
data/canada.json         410 MB/s   370 MB/s   620 MB/s   280 MB/s
data/citm_catalog.json  1060 MB/s   320 MB/s  1410 MB/s   380 MB/s
data/twitter.json        900 MB/s   410 MB/s   900 MB/s   370 MB/s
data/log.json           1080 MB/s   720 MB/s  1080 MB/s   720 MB/s

this

======= simd_json ======== parse|stringify ===== parse|stringify ====
data/canada.json         530 MB/s   430 MB/s   710 MB/s   320 MB/s
data/citm_catalog.json  1130 MB/s   480 MB/s  1550 MB/s   610 MB/s
data/twitter.json        920 MB/s   600 MB/s  1000 MB/s   650 MB/s
data/log.json           1080 MB/s  1080 MB/s  1080 MB/s  1080 MB/s
Licenser added 2 commits Dec 1, 2019
Hmm
@Licenser

This comment has been minimized.

Copy link
Member Author

Licenser commented Dec 1, 2019

0.2 release

  • upstream tracking version
  • generalisation of utf8parsing
  • generalisation of stage1
  • api update
  • perf improvements
  • tape parsing
@Licenser Licenser merged commit 0deaa23 into master Dec 1, 2019
35 checks passed
35 checks passed
clippy_check (-C target-cpu=native)
Details
build (stable, ubuntu-latest, -C target-cpu=native)
Details
clippy_check (-C target-cpu=native -C target-feature=-avx2)
Details
build (stable, ubuntu-latest, -C target-cpu=native, --features known-key)
Details
build (stable, ubuntu-latest, -C target-cpu=native -C target-feature=-avx2)
Details
build (stable, ubuntu-latest, -C target-cpu=native -C target-feature=-avx2, --features known-key)
Details
build (stable, windows-latest, -C target-cpu=native)
Details
build (stable, windows-latest, -C target-cpu=native, --features known-key)
Details
build (stable, windows-latest, -C target-cpu=native -C target-feature=-avx2)
Details
build (stable, windows-latest, -C target-cpu=native -C target-feature=-avx2, --features known-key)
Details
build (stable, macOS-latest, -C target-cpu=native)
Details
build (stable, macOS-latest, -C target-cpu=native, --features known-key)
Details
build (stable, macOS-latest, -C target-cpu=native -C target-feature=-avx2)
Details
build (stable, macOS-latest, -C target-cpu=native -C target-feature=-avx2, --features known-key)
Details
build (nightly, ubuntu-latest, -C target-cpu=native)
Details
build (nightly, ubuntu-latest, -C target-cpu=native, --features known-key)
Details
build (nightly, ubuntu-latest, -C target-cpu=native -C target-feature=-avx2)
Details
build (nightly, ubuntu-latest, -C target-cpu=native -C target-feature=-avx2, --features known-key)
Details
build (nightly, windows-latest, -C target-cpu=native)
Details
build (nightly, windows-latest, -C target-cpu=native, --features known-key)
Details
build (nightly, windows-latest, -C target-cpu=native -C target-feature=-avx2)
Details
build (nightly, windows-latest, -C target-cpu=native -C target-feature=-avx2, --features known-key)
Details
build (nightly, macOS-latest, -C target-cpu=native)
Details
build (nightly, macOS-latest, -C target-cpu=native, --features known-key)
Details
build (nightly, macOS-latest, -C target-cpu=native -C target-feature=-avx2)
Details
build (nightly, macOS-latest, -C target-cpu=native -C target-feature=-avx2, --features known-key)
Details
tarpaulin-sse
Details
tarpaulin-avx2
Details
tarpaulin-sse-known-key
Details
tarpaulin-avx2-known-key
Details
clippy clippy
Details
codecov/patch 86.44% of diff hit (target 82.23%)
Details
codecov/project 84.6% (+2.36%) compared to e1f67cb
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/drone/push Build is passing
Details
@Licenser Licenser moved this from To Do to D in 0.2 Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
0.2
  
D
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.