Replace JSON parser with RapidJSON #7708

lpereira · 2019-08-13T23:07:40Z

This gets rid of the Casablanca JSON parser and replaces it with RapidJSON, which is used in in-situ mode. This reduces temporary allocations quite significantly; so much so that, in my test, this shaves ~5.5ms from startup time of a webapp ASP.NET program.

This is a draft PR mostly to gather comments around portability issues (e.g. string vs. wide strings), and possibly some gotchas that I may have missed when converting the code to use this other library. The APIs are very similar, so it shouldn't be difficult to compare the diff.

RapidJSON is based off of commit d87b698, as there has not been a release in a few years now. I have not tested it with SIMD support, but since that's a compile-time option, I don't know if it's a good idea to use it (maybe SSE2 on x64, as that's part of the ISA, but we can't enable SSE4.2 for everybody; in any case, I'd have to measure to see if it's worth the trouble).

AaronRobinsonMSFT · 2019-08-13T23:13:31Z

@lpereira Can you also provide the binary size comparison for both with Casablanca and with RapidJSON. The startup win is awesome!

/cc @jeffschwMSFT

AaronRobinsonMSFT

Just a first pass.

AaronRobinsonMSFT · 2019-08-13T23:15:36Z

src/corehost/cli/comhost/clsidmap.cpp

-            json::object &val = prop.second.as_object();
-            e.assembly = val.at(_X("assembly")).as_string();
-            e.type = val.at(_X("type")).as_string();
+            auto &val = prop.value.GetObject();


I am a huge fan of auto when the type is observed on the right side or when using iterators. In this case, the type is opaque from usage and difficult to determine in review. I personally am okay with this but other might have a different opinion, just an FYI.

Personally, I'm a fan of the explicit type.

src/corehost/cli/fxr/hostpolicy_resolver.cpp

src/corehost/cli/json.h

lpereira · 2019-08-13T23:47:20Z

@lpereira Can you also provide the binary size comparison for both with Casablanca and with RapidJSON. The startup win is awesome!

These are the sizes with RapidJSON (release mode, Linux, x86-64, stripped):

   text	   data	    bss	    dec	    hex	filename
 306034	   2292	   1056	 309382	  4b886	libhostfxr.so
 258763	   2612	    800	 262175	  4001f	libhostpolicy.so

And these are the sizes with Casablanca (release mode, Linux, x86-64, stripped):

   text	   data	    bss	    dec	    hex	filename
 340175	   4340	   1088	 345603	  54603	libhostfxr.so
 294478	   4636	    840	 299954	  493b2	libhostpolicy.so

Which is pretty nifty, too: ~69KB of text + ~9KB of data shaved, for ~79KB of gain in size.

lpereira · 2019-08-14T00:10:09Z

Force-pushed to fix a build issue (it was not picking up the copy of RapidJSON in the repository; I have it installed as part of the system so the build was working just fine but broke in CI), and addressed some of the comments by @AaronRobinsonMSFT (thanks!)

elinor-fung

How hard would it be to also get time/size diffs on Windows? 😃

src/corehost/cli/winrthost/CMakeLists.txt

elinor-fung · 2019-08-14T05:10:22Z

src/corehost/cli/comhost/clsidmap.cpp

-            json::object &val = prop.second.as_object();
-            e.assembly = val.at(_X("assembly")).as_string();
-            e.type = val.at(_X("type")).as_string();
+            auto &val = prop.value.GetObject();


Personally, I'm a fan of the explicit type.

src/corehost/cli/json.h

src/corehost/cli/deps_format.h

src/corehost/cli/comhost/clsidmap.cpp

vitek-karas · 2019-08-14T10:56:56Z

We'll also need to add attribution to the https://github.com/dotnet/core-setup/blob/master/THIRD-PARTY-NOTICES.TXT (or maybe elsewhere - I'm not sure exactly how this works)

lpereira · 2019-08-14T16:54:48Z

We'll also need to add attribution to the https://github.com/dotnet/core-setup/blob/master/THIRD-PARTY-NOTICES.TXT (or maybe elsewhere - I'm not sure exactly how this works)

Absolutely -- I'll also add a README file pointing to RapidJSON, with information about which commit hash I obtained the files from, this kind of stuff.

lpereira · 2019-08-15T00:44:38Z

(Force-pushed with some changes, addressing some of the comments here -- but mostly to check if this will actually build on Windows.)

src/corehost/cli/comhost/clsidmap.cpp

src/corehost/cli/json_parser.cpp

src/corehost/cli/json_parser.h

lpereira · 2019-08-29T19:24:58Z

Force-pushed after cleaning up the encoding story: data is read from the file as UTF-8, but processed as UTF-16 on Windows. This unfortunately means that we can't do in-situ parsing on Windows at this point; we might be able to convert the UTF-8 data to UTF-16 and parse that in-situ, but I'll probably do that later. At this point I'm only looking towards making this work on Windows too, things can be improved later.

lpereira · 2019-08-29T19:29:14Z

@elinor-fung wrote:

How hard would it be to also get time/size diffs on Windows? smiley

Now that this is building on Windows, it should be possible to get the size diff with ease. I imagine performance will be slightly better, but not as good as on non-Windows platforms due to in-situ parsing not being used, though.

lpereira · 2019-08-29T20:45:32Z

The test failures are expected: they're testing malformed JSON files, and output has changed. Will need to change the tests themselves.

lpereira · 2019-08-30T23:24:05Z

Finally all tests are passing on Windows.

src/test/HostActivation.Tests/DependencyResolution/ResolveComponentDependencies.cs

src/test/HostActivation.Tests/CommandExtensions.cs

lpereira · 2019-09-03T18:06:37Z

Rebased, force-pushed, and addressed comments by @vitek-karas.

src/corehost/cli/json_parser.cpp

src/test/HostActivation.Tests/CommandExtensions.cs

src/corehost/cli/deps_format.cpp

src/corehost/cli/json_parser.cpp

If this variable is set in your environment, running `build.sh -t` will pass it to the test harness, causing these tests to fail. Teach Command.EnvironmentVariable() to handle `null` as an empty string, so that if the DotNetRoot() extension method is called with a `null` parameter, the $DOTNET_ROOT and $DOTNET_ROOT(x86) environment variables are essentially unset as far as pal::getenv() is concerned.

This decreases the host startup performance by a few milliseconds (at the order of 5ms) at the same time that the host libraries are reduced by almost 80KiB in size on a release build for x86-64/Linux. For comparison, these are the sizes with RapidJSON (release mode, Linux, x86-64, stripped): text data bss dec hex filename 306034 2292 1056 309382 4b886 libhostfxr.so 258763 2612 800 262175 4001f libhostpolicy.so And these are the sizes with Casablanca (release mode, Linux, x86-64, stripped): text data bss dec hex filename 340175 4340 1088 345603 54603 libhostfxr.so 294478 4636 840 299954 493b2 libhostpolicy.so RapidJSON is based off of commit d87b698d0fcc10a5f632ecbc80a9cb2a8fa094a5.

lpereira · 2019-09-05T17:48:24Z

Force-pushed addressing comments.

elinor-fung

I'd still be interested in seeing the Windows sizes / times too.

lpereira · 2019-09-05T20:57:20Z

I'd still be interested in seeing the Windows sizes / times too.

If I did everything right, here are the sizes (used size from mingw in WSL). Same deal, Windows, x86-64, release, stripped binaries. Before:

   text    data     bss     dec     hex filename
546434   14504       0  560938   88f2a hostpolicy.dll
554414   11952       0  566366   8a45e hostfxr.dll

After:

   text    data     bss     dec     hex filename
530634   10928       0  541562   8437a hostfxr.dll
523864   13480       0  537344   83300 hostpolicy.dll

So there's some size reduction on Windows as well, although not quite as dramatic (~48KB overall).

The timing difference is less dramatic, clocking in at ~1.8ms. I imagined that it wouldn't be in the same ballpark as on Linux, due to it not using the in-situ parsing mode and the required conversion, but it's still a win. (I might be making some rookie mistake when measuring this on Windows, though.)

lpereira requested review from vitek-karas, AaronRobinsonMSFT and elinor-fung August 13, 2019 23:07

AaronRobinsonMSFT reviewed Aug 13, 2019

View reviewed changes

lpereira force-pushed the rapidjson-master branch from 17dcf4f to 7c00832 Compare August 14, 2019 00:07

elinor-fung reviewed Aug 14, 2019

View reviewed changes

vitek-karas suggested changes Aug 14, 2019

View reviewed changes

src/corehost/cli/comhost/clsidmap.cpp Show resolved Hide resolved

lpereira force-pushed the rapidjson-master branch from 7c00832 to 955c3ae Compare August 15, 2019 00:43

AaronRobinsonMSFT reviewed Aug 15, 2019

View reviewed changes

src/corehost/cli/comhost/clsidmap.cpp Outdated Show resolved Hide resolved

src/corehost/cli/json_parser.cpp Outdated Show resolved Hide resolved

src/corehost/cli/json_parser.h Outdated Show resolved Hide resolved

lpereira force-pushed the rapidjson-master branch from 955c3ae to 77984c9 Compare August 29, 2019 19:21

lpereira force-pushed the rapidjson-master branch from 77984c9 to 8425188 Compare August 29, 2019 19:41

lpereira force-pushed the rapidjson-master branch 2 times, most recently from 5eef42e to 49004f0 Compare August 30, 2019 22:52

lpereira marked this pull request as ready for review August 30, 2019 23:38

vitek-karas reviewed Sep 2, 2019

View reviewed changes

src/test/HostActivation.Tests/DependencyResolution/ResolveComponentDependencies.cs Outdated Show resolved Hide resolved

src/test/HostActivation.Tests/CommandExtensions.cs Outdated Show resolved Hide resolved

lpereira force-pushed the rapidjson-master branch from 49004f0 to 5a73dcd Compare September 3, 2019 18:06

lpereira force-pushed the rapidjson-master branch from 5a73dcd to 0118ad1 Compare September 3, 2019 18:10

vitek-karas reviewed Sep 3, 2019

View reviewed changes

src/corehost/cli/json_parser.cpp Outdated Show resolved Hide resolved

lpereira force-pushed the rapidjson-master branch from 0118ad1 to eddf60d Compare September 3, 2019 21:31

vitek-karas approved these changes Sep 4, 2019

View reviewed changes

src/corehost/cli/json_parser.cpp Show resolved Hide resolved

elinor-fung reviewed Sep 4, 2019

View reviewed changes

src/test/HostActivation.Tests/CommandExtensions.cs Outdated Show resolved Hide resolved

src/corehost/cli/deps_format.cpp Show resolved Hide resolved

src/corehost/cli/json_parser.cpp Show resolved Hide resolved

lpereira added 2 commits September 5, 2019 10:38

lpereira force-pushed the rapidjson-master branch from eddf60d to 55fc6a9 Compare September 5, 2019 17:41

elinor-fung approved these changes Sep 5, 2019

View reviewed changes

lpereira merged commit 2706387 into dotnet:master Sep 6, 2019

am11 mentioned this pull request Sep 27, 2021

[Performance Proposal] Use SIMDJson in Corehost dotnet/runtime#59642

Open

am11 mentioned this pull request Feb 17, 2024

Update RapidJSON dotnet/runtime#98573

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace JSON parser with RapidJSON #7708

Replace JSON parser with RapidJSON #7708

lpereira commented Aug 13, 2019

AaronRobinsonMSFT commented Aug 13, 2019

AaronRobinsonMSFT left a comment

AaronRobinsonMSFT Aug 13, 2019

elinor-fung Aug 14, 2019

lpereira commented Aug 13, 2019 •

edited

Loading

lpereira commented Aug 14, 2019

elinor-fung left a comment

elinor-fung Aug 14, 2019

vitek-karas commented Aug 14, 2019

lpereira commented Aug 14, 2019

lpereira commented Aug 15, 2019

lpereira commented Aug 29, 2019

lpereira commented Aug 29, 2019

lpereira commented Aug 29, 2019

lpereira commented Aug 30, 2019

lpereira commented Sep 3, 2019

lpereira commented Sep 5, 2019

elinor-fung left a comment

lpereira commented Sep 5, 2019 •

edited

Loading

Replace JSON parser with RapidJSON #7708

Replace JSON parser with RapidJSON #7708

Conversation

lpereira commented Aug 13, 2019

AaronRobinsonMSFT commented Aug 13, 2019

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

AaronRobinsonMSFT Aug 13, 2019

Choose a reason for hiding this comment

elinor-fung Aug 14, 2019

Choose a reason for hiding this comment

lpereira commented Aug 13, 2019 • edited Loading

lpereira commented Aug 14, 2019

elinor-fung left a comment

Choose a reason for hiding this comment

elinor-fung Aug 14, 2019

Choose a reason for hiding this comment

vitek-karas commented Aug 14, 2019

lpereira commented Aug 14, 2019

lpereira commented Aug 15, 2019

lpereira commented Aug 29, 2019

lpereira commented Aug 29, 2019

lpereira commented Aug 29, 2019

lpereira commented Aug 30, 2019

lpereira commented Sep 3, 2019

lpereira commented Sep 5, 2019

elinor-fung left a comment

Choose a reason for hiding this comment

lpereira commented Sep 5, 2019 • edited Loading

lpereira commented Aug 13, 2019 •

edited

Loading

lpereira commented Sep 5, 2019 •

edited

Loading