Make WriteCommentValue() overloads have same behavior.

[lateapexearlyspeed]

Fix #52408 and #75075 Will add related tests after deciding correct format, thanks !

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details

---

null

Author:	lateapexearlyspeed
Assignees:	-
Labels:	area-System.Text.Json
Milestone:	-

[lateapexearlyspeed]

Hi @krwq just confirm, I will change code to implement just proposal description and align one overload behavior issue, and keep all other existing behavior no change.

I have pushed one version of prod code, and will continue adding tests later. If possible, maybe you can review and point out issue in advance, thanks !

[lateapexearlyspeed]

Hi @krwq Tests added, please review, thanks!

They used newtonsoft.json to do assertion as existing cases, but have to add some compensation newlines and whitespaces so that can verify new cases. Note the compensations are not entirely for new proposal, partly it is because we lost some test cases for existing indented behaviors of comment previously.

[eiriktsarpalis]

Could you try rebasing your branch? There appear to be merge conflicts

[lateapexearlyspeed]

Could you try rebasing your branch? There appear to be merge conflicts

Resolved, thanks !

[krwq]

/azp run

[azure-pipelines[bot]]

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

[krwq]

/azp run runtime

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[lateapexearlyspeed]

Hi @eiriktsarpalis @krwq, just kindly remind we had fixed or explained for all comments. Please check if anything missed to do :)

[krwq]

Any chance you could run perf tests on this change? There are changes made to the hot path so it would be useful to know if we regressed something or not and if yes how can we mitigate this

[lateapexearlyspeed]

Let me know if you're planning to fix this or not

Changed, please check.

Any chance you could run perf tests

I would try to run perf test after deciding current json format is ok, please help confirm, thanks @krwq !

[lateapexearlyspeed]

@krwq I run perf tests under Utf8JsonWriter, slower/faster result of perf test is not very stable, following is one sample: dotnet run --base "C:\results\before" --diff "C:\results\after" --threshold 2% summary: better: 6, geomean: 1.046 worse: 4, geomean: 1.057 total diff: 10

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Text.Json.Tests.Writer.Perf_Strings.WriteStringsUtf8(Formatted: False, Sk	1.08	5282410.64	5689104.17
System.Text.Json.Tests.Writer.Perf_Guids.WriteGuids(Formatted: True, SkipValidat	1.06	1934436.72	2054671.88
System.Text.Json.Tests.Writer.Perf_Basic.WriteBasicUtf16(Formatted: True, SkipVa	1.05	1285166.32	1352369.29
System.Text.Json.Tests.Writer.Perf_Strings.WriteStringsUtf16(Formatted: False, S	1.04	28113075.00	29187925.00

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Text.Json.Tests.Writer.Perf_Strings.WriteStringsUtf16(Formatted: True, Sk	1.07	7557617.65	7081003.13
System.Text.Json.Tests.Writer.Perf_Basic.WriteBasicUtf8(Formatted: True, SkipVal	1.05	381.68	362.26
System.Text.Json.Tests.Writer.Perf_Basic.WriteBasicUtf16(Formatted: False, SkipV	1.05	1134185.27	1076607.92
System.Text.Json.Tests.Writer.Perf_Basic.WriteBasicUtf16(Formatted: False, SkipV	1.04	1028596.29	990623.24
System.Text.Json.Tests.Writer.Perf_Base64.WriteByteArrayAsBase64_NoEscaping(Numb	1.03	189.53	183.39
System.Text.Json.Tests.Writer.Perf_Base64.WriteByteArrayAsBase64_HeavyEscaping(N	1.03	183.19	178.23

And run perf test for "worst" case System.Text.Json.Tests.Writer.Perf_Strings.WriteStringsUtf8 as above sample (note it is not always bad one), and found it has no obvious regression: BenchmarkDotNet=v0.13.1.1603-nightly, OS=Windows 10.0.22621 12th Gen Intel Core i7-12800H, 1 CPU, 20 logical and 14 physical cores .NET SDK=6.0.402 [Host] : .NET 6.0.10 (6.0.1022.47605), X64 RyuJIT Job-FHXNQJ : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT Job-HJTGLI : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1

Method	Job	Toolchain	Formatted	SkipValidation	Escaped	Mean	Error	StdDev	Median	Min	Max	Ratio	MannWhitney(2%)	RatioSD	Allocated
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	False	False	AllEscaped	24.862 ms	0.4697 ms	0.5025 ms	24.809 ms	24.186 ms	25.897 ms	0.99	Same	0.03	288 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	False	False	AllEscaped	25.110 ms	0.5516 ms	0.6353 ms	24.949 ms	24.178 ms	26.502 ms	1.00	Base	0.00	288 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	False	False	OneEscaped	5.549 ms	0.1038 ms	0.1019 ms	5.560 ms	5.339 ms	5.746 ms	1.00	Same	0.03	135 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	False	False	OneEscaped	5.572 ms	0.1085 ms	0.1250 ms	5.592 ms	5.211 ms	5.750 ms	1.00	Base	0.00	176 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	False	False	NoneEscaped	3.123 ms	0.0517 ms	0.0483 ms	3.141 ms	3.040 ms	3.180 ms	1.00	Same	0.03	124 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	False	False	NoneEscaped	3.120 ms	0.0617 ms	0.0606 ms	3.095 ms	3.028 ms	3.229 ms	1.00	Base	0.00	124 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	False	True	AllEscaped	24.777 ms	0.5306 ms	0.6110 ms	24.507 ms	23.961 ms	25.910 ms	0.99	Same	0.04	288 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	False	True	AllEscaped	25.188 ms	0.4922 ms	0.5055 ms	25.261 ms	24.368 ms	26.062 ms	1.00	Base	0.00	288 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	False	True	OneEscaped	5.390 ms	0.1068 ms	0.0999 ms	5.390 ms	5.237 ms	5.533 ms	1.02	Same	0.03	140 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	False	True	OneEscaped	5.304 ms	0.1031 ms	0.1146 ms	5.336 ms	5.035 ms	5.477 ms	1.00	Base	0.00	135 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	False	True	NoneEscaped	3.068 ms	0.0584 ms	0.0573 ms	3.089 ms	2.961 ms	3.132 ms	0.99	Same	0.02	124 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	False	True	NoneEscaped	3.081 ms	0.0349 ms	0.0309 ms	3.085 ms	3.000 ms	3.124 ms	1.00	Base	0.00	124 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	True	False	AllEscaped	24.923 ms	0.3265 ms	0.3054 ms	24.892 ms	24.620 ms	25.546 ms	0.99	Same	0.03	288 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	True	False	AllEscaped	25.069 ms	0.4837 ms	0.5176 ms	25.012 ms	24.214 ms	26.047 ms	1.00	Base	0.00	288 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	True	False	OneEscaped	5.655 ms	0.0670 ms	0.0627 ms	5.671 ms	5.481 ms	5.742 ms	1.00	Same	0.02	135 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	True	False	OneEscaped	5.658 ms	0.1129 ms	0.1255 ms	5.630 ms	5.438 ms	5.824 ms	1.00	Base	0.00	136 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	True	False	NoneEscaped	3.325 ms	0.0441 ms	0.0391 ms	3.322 ms	3.244 ms	3.392 ms	0.97	Same	0.02	125 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	True	False	NoneEscaped	3.418 ms	0.0603 ms	0.0592 ms	3.410 ms	3.341 ms	3.562 ms	1.00	Base	0.00	124 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	True	True	AllEscaped	24.961 ms	0.3017 ms	0.2356 ms	24.953 ms	24.644 ms	25.529 ms	0.98	Same	0.02	288 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	True	True	AllEscaped	25.318 ms	0.4664 ms	0.4134 ms	25.259 ms	24.831 ms	26.301 ms	1.00	Base	0.00	288 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	True	True	OneEscaped	5.746 ms	0.0776 ms	0.0726 ms	5.731 ms	5.581 ms	5.857 ms	1.00	Same	0.02	136 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	True	True	OneEscaped	5.722 ms	0.0883 ms	0.0826 ms	5.729 ms	5.586 ms	5.820 ms	1.00	Base	0.00	135 B
WriteStringsUtf8	Job-FHXNQJ	\8.0.0-after\CoreRun.exe	True	True	NoneEscaped	3.266 ms	0.0370 ms	0.0346 ms	3.265 ms	3.193 ms	3.323 ms	1.01	Same	0.02	124 B
WriteStringsUtf8	Job-HJTGLI	\8.0.0-before\CoreRun.exe	True	True	NoneEscaped	3.229 ms	0.0383 ms	0.0359 ms	3.242 ms	3.139 ms	3.273 ms	1.00	Base	0.00	128 B

[krwq]

/azp run runtime

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[krwq]

@lateapexearlyspeed given there are some issues with the CI I'll manually squash and rebase it. I haven't seen those issues in the recent builds

[krwq]

(I force pushed again because git changed author to me so I changed you back as the author by hand)

[krwq]

Thanks @lateapexearlyspeed!