Use System.Text.Json by DaniilSokolyuk · Pull Request #53 · JeringTech/Javascript.NodeJS

DaniilSokolyuk · 2019-11-24T00:08:34Z

Before

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.17763.475 (1809/October2018Update/Redstone5)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100
  [Host] : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT

Job=InProcess  Toolchain=InProcessEmitToolchain

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
INodeJSService_InvokeFromFile	181.0 us	1.20 us	1.12 us	2.9297	-	-	11.99 KB
INodeJSService_InvokeFromCache	179.5 us	2.23 us	1.74 us	2.9297	-	-	11.83 KB
INodeServices	186.3 us	2.12 us	1.88 us	2.4414	-	-	10.37 KB

After

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.17763.475 (1809/October2018Update/Redstone5)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100
  [Host] : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT

Job=InProcess  Toolchain=InProcessEmitToolchain

Method	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated
INodeJSService_InvokeFromFile	181.5 us	4.07 us	10.13 us	177.1 us	0.9766	-	-	5.8 KB
INodeJSService_InvokeFromCache	171.0 us	2.41 us	2.25 us	170.3 us	1.2207	-	-	5.65 KB
INodeServices	190.3 us	2.10 us	1.86 us	189.5 us	2.4414	-	-	10.37 KB

Here one more benchmark
DaniilSokolyuk/NodeReact.NET#3

JeremyTCD · 2019-11-24T04:27:15Z

Hey looks good! I need access to the pull request branch, thanks.

DaniilSokolyuk · 2019-11-24T07:36:20Z

@JeremyTCD Done

codecov · 2019-11-24T08:23:04Z

Codecov Report

Merging #53 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   97.84%   97.77%   -0.08%     
==========================================
  Files          17       17              
  Lines         511      494      -17     
==========================================
- Hits          500      483      -17     
  Misses         11       11

Impacted Files	Coverage Δ
src/NodeJS/InvocationData/InvocationRequest.cs	`100% <ø> (ø)`	⬆️
src/NodeJS/InvocationData/InvocationError.cs	`100% <100%> (ø)`	⬆️
...ementations/OutOfProcess/Http/HttpNodeJSService.cs	`96.61% <100%> (-0.12%)`	⬇️
...ementations/OutOfProcess/Http/InvocationContent.cs	`100% <100%> (ø)`	⬆️
src/NodeJS/Utils/JsonService.cs	`100% <100%> (ø)`	⬆️

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 4661c99...4e42e34. Read the comment docs.

- Added ConfigureAwait(false). - Minor formatting, Stream is disposable, to be safe and consistent wrap in using block.

- InvocationError can't be immutable since System.Text.Json doesn't support private setters yet, looks like support will be added soon - https://github.com/dotnet/corefx/issues/38163#issuecomment-553152589. Review when System.Text.Json 5.0.0 is released. - Not a blocking issue since this library doesn't actually use generated InvocationError instances, simply passes them to user.

- moduleSourceType can only be ModuleSourceType.Stream, ModuleSourceType.String or ModuleSourceType.File.

JeremyTCD

Almost ready to merge, just unsure about a couple of things.

perf/NodeJS/Benchmarks.cs

perf/NodeJS/Jering.Javascript.NodeJS.Performance.csproj

src/NodeJS/Jering.Javascript.NodeJS.csproj

DaniilSokolyuk · 2019-11-25T10:19:33Z

@JeremyTCD
Also now deserializing is truly async, because JsonTextReader read stream synchronously, this can reduce potential thread-pool starvation

…e.js stdout/stderr flushing issue

- Latency benchmarks use minimal JSON, so they do not reflect the improvement in JSON serialization/deserialization performance. Refer to benchmarks in this commit's PR for a clearer picture.

JeremyTCD · 2019-11-25T10:51:43Z

Good point!

Please add your details to the ReadMe under "Contributors" and your node project under "Projects Using this Library".

DaniilSokolyuk · 2019-11-25T11:08:46Z

@JeremyTCD Done

- Benchmark couldn't find javascript module after cleaning solution.

JeremyTCD · 2019-11-25T11:46:49Z

🚀 Releasing v5.0.0, bumped major version because of changes to IJsonService. Thanks for the contribution!

Switched from Newtonsoft.Json to System.Text.Json.

Daniil Sokolyuk and others added 9 commits November 25, 2019 15:42

System.Text.Json

0756441

clean

1e0209f

use LTS deps

bca5da6

Cleaned up XML comments for JsonService.

70e1c23

HttpNodeJSService cleanup

2496c80

- Added ConfigureAwait(false). - Minor formatting, Stream is disposable, to be safe and consistent wrap in using block.

Minor formatting in InvocationContent.cs.

47d4b21

Minor tweaks to HttpNodeJSServiceUnitTests.cs.

505b83e

XML comments in InvocationRequest

7426427

- moduleSourceType can only be ModuleSourceType.Stream, ModuleSourceType.String or ModuleSourceType.File.

JeremyTCD reviewed Nov 25, 2019

View reviewed changes

perf/NodeJS/Benchmarks.cs Outdated Show resolved Hide resolved

perf/NodeJS/Jering.Javascript.NodeJS.Performance.csproj Outdated Show resolved Hide resolved

src/NodeJS/Jering.Javascript.NodeJS.csproj Show resolved Hide resolved

Rolled back changes in benchmark

56c7a69

JeremyTCD added 3 commits November 25, 2019 18:24

Removed redundant configure.

4d5237f

Minor change to HttpNodeJSServiceIntegrationTests.cs to deal with Nod…

95c973d

…e.js stdout/stderr flushing issue

Updated ReadMe

84cccb4

- Latency benchmarks use minimal JSON, so they do not reflect the improvement in JSON serialization/deserialization performance. Refer to benchmarks in this commit's PR for a clearer picture.

Added release 5.0.0.

108bee0

DaniilSokolyuk and others added 2 commits November 25, 2019 19:39

Update readme

2525370

Reverted Benchmarks.cs and Jering.Javascript.NodeJS.Performance.csproj

4e42e34

- Benchmark couldn't find javascript module after cleaning solution.

JeremyTCD merged commit 5945488 into JeringTech:master Nov 25, 2019

JeremyTCD added a commit that referenced this pull request Nov 25, 2019

Merge pull request #53 from DaniilSokolyuk/reduceallocations

eb90da8

Switched from Newtonsoft.Json to System.Text.Json.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use System.Text.Json#53

Use System.Text.Json#53
JeremyTCD merged 16 commits intoJeringTech:masterfrom
DaniilSokolyuk:reduceallocations

DaniilSokolyuk commented Nov 24, 2019

Uh oh!

JeremyTCD commented Nov 24, 2019

Uh oh!

DaniilSokolyuk commented Nov 24, 2019

Uh oh!

codecov bot commented Nov 24, 2019 •

edited

Loading

Uh oh!

JeremyTCD left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaniilSokolyuk commented Nov 25, 2019

Uh oh!

JeremyTCD commented Nov 25, 2019 •

edited

Loading

Uh oh!

DaniilSokolyuk commented Nov 25, 2019

Uh oh!

JeremyTCD commented Nov 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

DaniilSokolyuk commented Nov 24, 2019

Uh oh!

JeremyTCD commented Nov 24, 2019

Uh oh!

DaniilSokolyuk commented Nov 24, 2019

Uh oh!

codecov bot commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JeremyTCD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaniilSokolyuk commented Nov 25, 2019

Uh oh!

JeremyTCD commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaniilSokolyuk commented Nov 25, 2019

Uh oh!

JeremyTCD commented Nov 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Nov 24, 2019 •

edited

Loading

JeremyTCD commented Nov 25, 2019 •

edited

Loading