Skip to content

Remove the second type parameter from CastingConverter - #80755

Merged
eiriktsarpalis merged 1 commit into
dotnet:mainfrom
eiriktsarpalis:castingconverter-remove-generic-parameter
Jan 20, 2023
Merged

Remove the second type parameter from CastingConverter#80755
eiriktsarpalis merged 1 commit into
dotnet:mainfrom
eiriktsarpalis:castingconverter-remove-generic-parameter

Conversation

@eiriktsarpalis

@eiriktsarpaliseiriktsarpalis commented Jan 17, 2023•
edited
Loading

Copy link
Copy Markdown
Member

The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:

System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>.

This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.

cc @eerhardt@jkotas

@eiriktsarpaliseiriktsarpalis added this to the 8.0.0 milestone Jan 17, 2023
@ghost

ghost commented Jan 17, 2023

Copy link
Copy Markdown

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

The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:

System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>.

This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.

cc @eerhardt

Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:

area-System.Text.Json

Milestone:8.0.0

eiriktsarpalis
eiriktsarpalis commented Jan 17, 2023
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs

internal abstract bool OnTryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
internal abstract bool TryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
internal abstract object? ReadAsObject(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options);

@eiriktsarpaliseiriktsarpalisJan 17, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this change is adding new methods to JsonConverter this should increase code size linearly over the number of generic specializations. This should still contribute to a net decrease of code size given that CastingConverter contributes to quadratic growth over the number of types being used. I will follow up with real numbers validating this hypothesis.

@eerhardt

eerhardt commented Jan 17, 2023

Copy link
Copy Markdown
Member

which is contributing to a quadratic explosion of generic specializations in NativeAOT:

Is it possible to get before/after size numbers for this change? If you need a test app, the one we are using in our perf measurements is here.

@build-analysisbuild-analysisBot mentioned this pull request Jan 18, 2023
Closed
@eiriktsarpalis

eiriktsarpalis commented Jan 18, 2023

Copy link
Copy Markdown
MemberAuthor

I compared the publish AOT size for the Goldilocks app between main and the PR branch. I'm seeing a ~7MB improvement in the size of the publish folder, from 120 MB down to 113 MB.

@eiriktsarpalis

eiriktsarpalis commented Jan 19, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

STJ Numbers from the mstat dump of the Goldilocks app. Seeing just over 1MB of size reduction overall:

21156 publish-main/Goldilocks
20064 publish-pr/Goldilocks

main

// ********** Types Total Size 3,327,268
System.Text.Json 476,148
// ********** Size By Namespace
System.Text.Json.Serialization.Converters 876,578
System.Text.Json.Serialization.Metadata 607,084
System.Text.Json.Serialization 361,833
System.Text.Json 203,356

PR

// ********** Types Total Size 3,172,132
System.Text.Json 320,884
// ********** Size By Namespace
System.Text.Json.Serialization.Metadata 605,298
System.Text.Json.Serialization 397,892
System.Text.Json 208,571
System.Text.Json.Serialization.Converters 200,477

@eerhardt

eerhardt commented Jan 19, 2023

Copy link
Copy Markdown
Member

/benchmark json aspnet-citrine-win libs

@pr-benchmarks

pr-benchmarksBot commented Jan 19, 2023

Copy link
Copy Markdown

Benchmark started for json on aspnet-citrine-win with libs. Logs: link

eerhardt
eerhardt approved these changes Jan 19, 2023
View reviewed changes

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look over the code (I'm no expert in this area), but I think this is a great change. If there are no runtime perf degradations, I think this is a great NativeAOT size win (5% of the new dotnet new api -aot app).

@pr-benchmarks

pr-benchmarksBot commented Jan 19, 2023

Copy link
Copy Markdown
json - aspnet-citrine-win

applicationjson.basejson.pr
CPU Usage (%)8087+8.75%
Cores usage (%)2,2292,433+9.15%
Working Set (MB)7374+1.37%
Private Memory (MB)100101+1.00%
Build Time (ms)4,9793,287-33.98%
Start Time (ms)417418+0.24%
Published Size (KB)96,70296,7020.00%
Symbols Size (KB)52520.00%
.NET Core SDK Version8.0.100-alpha.1.23069.68.0.100-alpha.1.23069.6
loadjson.basejson.pr
CPU Usage (%)7274+2.78%
Cores usage (%)2,0262,084+2.86%
Working Set (MB)1191190.00%
Private Memory (MB)3633630.00%
Start Time (ms)00
First Request (ms)85850.00%
Requests/sec1,059,9091,095,251+3.33%
Requests16,004,21016,537,175+3.33%
Mean latency (ms)0.891.11+24.72%
Max latency (ms)36.8459.44+61.35%
Bad responses00
Socket errors00
Read throughput (MB/s)147.58152.50+3.33%
Latency 50th (ms)0.320.31-4.64%
Latency 75th (ms)0.770.83+8.37%
Latency 90th (ms)2.343.10+32.48%
Latency 99th (ms)7.0210.31+46.87%

krwq
krwq approved these changes Jan 20, 2023
View reviewed changes

@krwqkrwq left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome win, thanks!

@krwq

krwq commented Jan 20, 2023

Copy link
Copy Markdown
Member

:shipit:

@krwqkrwq added the size-reduction Issues impacting final app size primary for size sensitive workloads label Jan 20, 2023
@ghost

ghost commented Jan 20, 2023

Copy link
Copy Markdown

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:

System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>.

This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.

cc @eerhardt@jkotas

Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:

area-System.Text.Json, size-reduction

Milestone:8.0.0

@eiriktsarpaliseiriktsarpalis mentioned this pull request Jan 20, 2023
Open
@eiriktsarpalis
eiriktsarpalis merged commit a04aaeb into dotnet:mainJan 20, 2023
@eiriktsarpalis
eiriktsarpalis deleted the castingconverter-remove-generic-parameter branch January 20, 2023 16:15
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
@eiriktsarpalis@mdh1418
Remove second type parameter from CastingConverter (dotnet#80755)
81e99ea
@ghostghost locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@krwqkrwqkrwq approved these changes
@eerhardteerhardteerhardt approved these changes
@layomialayomiaAwaiting requested review from layomia
@tarekghtarekghAwaiting requested review from tarekgh

Assignees

@eiriktsarpaliseiriktsarpalis

Labels

area-System.Text.Jsonsize-reductionIssues impacting final app size primary for size sensitive workloads

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@eiriktsarpalis@eerhardt@krwq
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Remove the second type parameter from CastingConverter by eiriktsarpalis · Pull Request #80755 · dotnet/runtime · GitHub
Skip to content
Remove the second type parameter from CastingConverter - #80755
Merged
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Jan 20, 2023
Merged
Remove the second type parameter from CastingConverter#80755
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Conversation
@eiriktsarpalis
@eiriktsarpaliseiriktsarpalis
commented
Jan 17, 2023•

edited

Loading

Copy link


Copy Markdown

Member
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
@eiriktsarpaliseiriktsarpalis
added this to the 8.0.0 milestone
Jan 17, 2023
@ghost
ghost
commented
Jan 17, 2023

Copy link


Copy Markdown

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
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json
Milestone:8.0.0
eiriktsarpalis
eiriktsarpalis
commented
Jan 17, 2023
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs

 internal abstract bool OnTryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract bool TryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract object? ReadAsObject(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options);
@eiriktsarpaliseiriktsarpalisJan 17, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this change is adding new methods to JsonConverter this should increase code size linearly over the number of generic specializations. This should still contribute to a net decrease of code size given that CastingConverter contributes to quadratic growth over the number of types being used. I will follow up with real numbers validating this hypothesis.
@eerhardt
eerhardt
commented
Jan 17, 2023

Copy link


Copy Markdown

Member
which is contributing to a quadratic explosion of generic specializations in NativeAOT:
Is it possible to get before/after size numbers for this change? If you need a test app, the one we are using in our perf measurements is here.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 18, 2023

Closed

@eiriktsarpalis
eiriktsarpalis
commented
Jan 18, 2023

Copy link


Copy Markdown

MemberAuthor
I compared the publish AOT size for the Goldilocks app between main and the PR branch. I'm seeing a ~7MB improvement in the size of the publish folder, from 120 MB down to 113 MB.
@eiriktsarpalis
eiriktsarpalis
commented
Jan 19, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
STJ Numbers from the mstat dump of the Goldilocks app. Seeing just over 1MB of size reduction overall:
21156 publish-main/Goldilocks
20064 publish-pr/Goldilocks

main
// ********** Types Total Size 3,327,268
System.Text.Json 476,148
// ********** Size By Namespace
System.Text.Json.Serialization.Converters 876,578
System.Text.Json.Serialization.Metadata 607,084
System.Text.Json.Serialization 361,833
System.Text.Json 203,356

PR
// ********** Types Total Size 3,172,132
System.Text.Json 320,884
// ********** Size By Namespace
System.Text.Json.Serialization.Metadata 605,298
System.Text.Json.Serialization 397,892
System.Text.Json 208,571
System.Text.Json.Serialization.Converters 200,477

@eerhardt
eerhardt
commented
Jan 19, 2023

Copy link


Copy Markdown

Member
/benchmark json aspnet-citrine-win libs
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

Benchmark started for json on aspnet-citrine-win with libs. Logs: link
eerhardt
eerhardt
approved these changes
Jan 19, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look over the code (I'm no expert in this area), but I think this is a great change. If there are no runtime perf degradations, I think this is a great NativeAOT size win (5% of the new dotnet new api -aot app).
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

json - aspnet-citrine-win
applicationjson.basejson.pr
CPU Usage (%)8087+8.75%
Cores usage (%)2,2292,433+9.15%
Working Set (MB)7374+1.37%
Private Memory (MB)100101+1.00%
Build Time (ms)4,9793,287-33.98%
Start Time (ms)417418+0.24%
Published Size (KB)96,70296,7020.00%
Symbols Size (KB)52520.00%
.NET Core SDK Version8.0.100-alpha.1.23069.68.0.100-alpha.1.23069.6
loadjson.basejson.pr
CPU Usage (%)7274+2.78%
Cores usage (%)2,0262,084+2.86%
Working Set (MB)1191190.00%
Private Memory (MB)3633630.00%
Start Time (ms)00
First Request (ms)85850.00%
Requests/sec1,059,9091,095,251+3.33%
Requests16,004,21016,537,175+3.33%
Mean latency (ms)0.891.11+24.72%
Max latency (ms)36.8459.44+61.35%
Bad responses00
Socket errors00
Read throughput (MB/s)147.58152.50+3.33%
Latency 50th (ms)0.320.31-4.64%
Latency 75th (ms)0.770.83+8.37%
Latency 90th (ms)2.343.10+32.48%
Latency 99th (ms)7.0210.31+46.87%
krwq
krwq
approved these changes
Jan 20, 2023
View reviewed changes
@krwqkrwq
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome win, thanks!
@krwq
krwq
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
:shipit:
@krwqkrwq
added
the 
size-reduction
Issues impacting final app size primary for size sensitive workloads
label
Jan 20, 2023
@ghost
ghost
commented
Jan 20, 2023

Copy link


Copy Markdown

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar

See info in area-owners.md if you want to be subscribed.
Issue Details
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json, size-reduction
Milestone:8.0.0
@eiriktsarpaliseiriktsarpalis
mentioned this pull request
Jan 20, 2023

Open

@eiriktsarpalis
eiriktsarpalis
merged commit a04aaeb
into
dotnet:mainJan 20, 2023
@eiriktsarpalis
eiriktsarpalis
deleted the

castingconverter-remove-generic-parameter

branch
January 20, 2023 16:15
mdh1418 pushed a commit
to mdh1418/runtime
that referenced
this pull request
Jan 24, 2023
@eiriktsarpalis@mdh1418
Remove second type parameter from CastingConverter (dotnet#80755)
81e99ea
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 19, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@krwqkrwqkrwq approved these changes
@eerhardteerhardteerhardt approved these changes
@layomialayomiaAwaiting requested review from layomia
@tarekghtarekghAwaiting requested review from tarekgh

Assignees

@eiriktsarpaliseiriktsarpalis

Labels

area-System.Text.Jsonsize-reductionIssues impacting final app size primary for size sensitive workloads

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@eiriktsarpalis@eerhardt@krwq

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Remove the second type parameter from CastingConverter by eiriktsarpalis · Pull Request #80755 · dotnet/runtime · GitHub
Skip to content
Remove the second type parameter from CastingConverter - #80755
Merged
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Jan 20, 2023
Merged
Remove the second type parameter from CastingConverter#80755
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Conversation
@eiriktsarpalis
@eiriktsarpaliseiriktsarpalis
commented
Jan 17, 2023•

edited

Loading

Copy link


Copy Markdown

Member
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
@eiriktsarpaliseiriktsarpalis
added this to the 8.0.0 milestone
Jan 17, 2023
@ghost
ghost
commented
Jan 17, 2023

Copy link


Copy Markdown

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
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json
Milestone:8.0.0
eiriktsarpalis
eiriktsarpalis
commented
Jan 17, 2023
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs

 internal abstract bool OnTryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract bool TryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract object? ReadAsObject(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options);
@eiriktsarpaliseiriktsarpalisJan 17, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this change is adding new methods to JsonConverter this should increase code size linearly over the number of generic specializations. This should still contribute to a net decrease of code size given that CastingConverter contributes to quadratic growth over the number of types being used. I will follow up with real numbers validating this hypothesis.
@eerhardt
eerhardt
commented
Jan 17, 2023

Copy link


Copy Markdown

Member
which is contributing to a quadratic explosion of generic specializations in NativeAOT:
Is it possible to get before/after size numbers for this change? If you need a test app, the one we are using in our perf measurements is here.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 18, 2023

Closed

@eiriktsarpalis
eiriktsarpalis
commented
Jan 18, 2023

Copy link


Copy Markdown

MemberAuthor
I compared the publish AOT size for the Goldilocks app between main and the PR branch. I'm seeing a ~7MB improvement in the size of the publish folder, from 120 MB down to 113 MB.
@eiriktsarpalis
eiriktsarpalis
commented
Jan 19, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
STJ Numbers from the mstat dump of the Goldilocks app. Seeing just over 1MB of size reduction overall:
21156 publish-main/Goldilocks
20064 publish-pr/Goldilocks

main
// ********** Types Total Size 3,327,268
System.Text.Json 476,148
// ********** Size By Namespace
System.Text.Json.Serialization.Converters 876,578
System.Text.Json.Serialization.Metadata 607,084
System.Text.Json.Serialization 361,833
System.Text.Json 203,356

PR
// ********** Types Total Size 3,172,132
System.Text.Json 320,884
// ********** Size By Namespace
System.Text.Json.Serialization.Metadata 605,298
System.Text.Json.Serialization 397,892
System.Text.Json 208,571
System.Text.Json.Serialization.Converters 200,477

@eerhardt
eerhardt
commented
Jan 19, 2023

Copy link


Copy Markdown

Member
/benchmark json aspnet-citrine-win libs
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

Benchmark started for json on aspnet-citrine-win with libs. Logs: link
eerhardt
eerhardt
approved these changes
Jan 19, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look over the code (I'm no expert in this area), but I think this is a great change. If there are no runtime perf degradations, I think this is a great NativeAOT size win (5% of the new dotnet new api -aot app).
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

json - aspnet-citrine-win
applicationjson.basejson.pr
CPU Usage (%)8087+8.75%
Cores usage (%)2,2292,433+9.15%
Working Set (MB)7374+1.37%
Private Memory (MB)100101+1.00%
Build Time (ms)4,9793,287-33.98%
Start Time (ms)417418+0.24%
Published Size (KB)96,70296,7020.00%
Symbols Size (KB)52520.00%
.NET Core SDK Version8.0.100-alpha.1.23069.68.0.100-alpha.1.23069.6
loadjson.basejson.pr
CPU Usage (%)7274+2.78%
Cores usage (%)2,0262,084+2.86%
Working Set (MB)1191190.00%
Private Memory (MB)3633630.00%
Start Time (ms)00
First Request (ms)85850.00%
Requests/sec1,059,9091,095,251+3.33%
Requests16,004,21016,537,175+3.33%
Mean latency (ms)0.891.11+24.72%
Max latency (ms)36.8459.44+61.35%
Bad responses00
Socket errors00
Read throughput (MB/s)147.58152.50+3.33%
Latency 50th (ms)0.320.31-4.64%
Latency 75th (ms)0.770.83+8.37%
Latency 90th (ms)2.343.10+32.48%
Latency 99th (ms)7.0210.31+46.87%
krwq
krwq
approved these changes
Jan 20, 2023
View reviewed changes
@krwqkrwq
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome win, thanks!
@krwq
krwq
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
:shipit:
@krwqkrwq
added
the 
size-reduction
Issues impacting final app size primary for size sensitive workloads
label
Jan 20, 2023
@ghost
ghost
commented
Jan 20, 2023

Copy link


Copy Markdown

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar

See info in area-owners.md if you want to be subscribed.
Issue Details
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json, size-reduction
Milestone:8.0.0
@eiriktsarpaliseiriktsarpalis
mentioned this pull request
Jan 20, 2023

Open

@eiriktsarpalis
eiriktsarpalis
merged commit a04aaeb
into
dotnet:mainJan 20, 2023
@eiriktsarpalis
eiriktsarpalis
deleted the

castingconverter-remove-generic-parameter

branch
January 20, 2023 16:15
mdh1418 pushed a commit
to mdh1418/runtime
that referenced
this pull request
Jan 24, 2023
@eiriktsarpalis@mdh1418
Remove second type parameter from CastingConverter (dotnet#80755)
81e99ea
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 19, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@krwqkrwqkrwq approved these changes
@eerhardteerhardteerhardt approved these changes
@layomialayomiaAwaiting requested review from layomia
@tarekghtarekghAwaiting requested review from tarekgh

Assignees

@eiriktsarpaliseiriktsarpalis

Labels

area-System.Text.Jsonsize-reductionIssues impacting final app size primary for size sensitive workloads

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@eiriktsarpalis@eerhardt@krwq

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Remove the second type parameter from CastingConverter by eiriktsarpalis · Pull Request #80755 · dotnet/runtime · GitHub
Skip to content
Remove the second type parameter from CastingConverter - #80755
Merged
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Jan 20, 2023
Merged
Remove the second type parameter from CastingConverter#80755
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Conversation
@eiriktsarpalis
@eiriktsarpaliseiriktsarpalis
commented
Jan 17, 2023•

edited

Loading

Copy link


Copy Markdown

Member
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
@eiriktsarpaliseiriktsarpalis
added this to the 8.0.0 milestone
Jan 17, 2023
@ghost
ghost
commented
Jan 17, 2023

Copy link


Copy Markdown

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
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json
Milestone:8.0.0
eiriktsarpalis
eiriktsarpalis
commented
Jan 17, 2023
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs

 internal abstract bool OnTryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract bool TryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract object? ReadAsObject(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options);
@eiriktsarpaliseiriktsarpalisJan 17, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this change is adding new methods to JsonConverter this should increase code size linearly over the number of generic specializations. This should still contribute to a net decrease of code size given that CastingConverter contributes to quadratic growth over the number of types being used. I will follow up with real numbers validating this hypothesis.
@eerhardt
eerhardt
commented
Jan 17, 2023

Copy link


Copy Markdown

Member
which is contributing to a quadratic explosion of generic specializations in NativeAOT:
Is it possible to get before/after size numbers for this change? If you need a test app, the one we are using in our perf measurements is here.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 18, 2023

Closed

@eiriktsarpalis
eiriktsarpalis
commented
Jan 18, 2023

Copy link


Copy Markdown

MemberAuthor
I compared the publish AOT size for the Goldilocks app between main and the PR branch. I'm seeing a ~7MB improvement in the size of the publish folder, from 120 MB down to 113 MB.
@eiriktsarpalis
eiriktsarpalis
commented
Jan 19, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
STJ Numbers from the mstat dump of the Goldilocks app. Seeing just over 1MB of size reduction overall:
21156 publish-main/Goldilocks
20064 publish-pr/Goldilocks

main
// ********** Types Total Size 3,327,268
System.Text.Json 476,148
// ********** Size By Namespace
System.Text.Json.Serialization.Converters 876,578
System.Text.Json.Serialization.Metadata 607,084
System.Text.Json.Serialization 361,833
System.Text.Json 203,356

PR
// ********** Types Total Size 3,172,132
System.Text.Json 320,884
// ********** Size By Namespace
System.Text.Json.Serialization.Metadata 605,298
System.Text.Json.Serialization 397,892
System.Text.Json 208,571
System.Text.Json.Serialization.Converters 200,477

@eerhardt
eerhardt
commented
Jan 19, 2023

Copy link


Copy Markdown

Member
/benchmark json aspnet-citrine-win libs
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

Benchmark started for json on aspnet-citrine-win with libs. Logs: link
eerhardt
eerhardt
approved these changes
Jan 19, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look over the code (I'm no expert in this area), but I think this is a great change. If there are no runtime perf degradations, I think this is a great NativeAOT size win (5% of the new dotnet new api -aot app).
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

json - aspnet-citrine-win
applicationjson.basejson.pr
CPU Usage (%)8087+8.75%
Cores usage (%)2,2292,433+9.15%
Working Set (MB)7374+1.37%
Private Memory (MB)100101+1.00%
Build Time (ms)4,9793,287-33.98%
Start Time (ms)417418+0.24%
Published Size (KB)96,70296,7020.00%
Symbols Size (KB)52520.00%
.NET Core SDK Version8.0.100-alpha.1.23069.68.0.100-alpha.1.23069.6
loadjson.basejson.pr
CPU Usage (%)7274+2.78%
Cores usage (%)2,0262,084+2.86%
Working Set (MB)1191190.00%
Private Memory (MB)3633630.00%
Start Time (ms)00
First Request (ms)85850.00%
Requests/sec1,059,9091,095,251+3.33%
Requests16,004,21016,537,175+3.33%
Mean latency (ms)0.891.11+24.72%
Max latency (ms)36.8459.44+61.35%
Bad responses00
Socket errors00
Read throughput (MB/s)147.58152.50+3.33%
Latency 50th (ms)0.320.31-4.64%
Latency 75th (ms)0.770.83+8.37%
Latency 90th (ms)2.343.10+32.48%
Latency 99th (ms)7.0210.31+46.87%
krwq
krwq
approved these changes
Jan 20, 2023
View reviewed changes
@krwqkrwq
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome win, thanks!
@krwq
krwq
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
:shipit:
@krwqkrwq
added
the 
size-reduction
Issues impacting final app size primary for size sensitive workloads
label
Jan 20, 2023
@ghost
ghost
commented
Jan 20, 2023

Copy link


Copy Markdown

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar

See info in area-owners.md if you want to be subscribed.
Issue Details
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json, size-reduction
Milestone:8.0.0
@eiriktsarpaliseiriktsarpalis
mentioned this pull request
Jan 20, 2023

Open

@eiriktsarpalis
eiriktsarpalis
merged commit a04aaeb
into
dotnet:mainJan 20, 2023
@eiriktsarpalis
eiriktsarpalis
deleted the

castingconverter-remove-generic-parameter

branch
January 20, 2023 16:15
mdh1418 pushed a commit
to mdh1418/runtime
that referenced
this pull request
Jan 24, 2023
@eiriktsarpalis@mdh1418
Remove second type parameter from CastingConverter (dotnet#80755)
81e99ea
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 19, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@krwqkrwqkrwq approved these changes
@eerhardteerhardteerhardt approved these changes
@layomialayomiaAwaiting requested review from layomia
@tarekghtarekghAwaiting requested review from tarekgh

Assignees

@eiriktsarpaliseiriktsarpalis

Labels

area-System.Text.Jsonsize-reductionIssues impacting final app size primary for size sensitive workloads

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@eiriktsarpalis@eerhardt@krwq

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Remove the second type parameter from CastingConverter by eiriktsarpalis · Pull Request #80755 · dotnet/runtime · GitHub
Skip to content
Remove the second type parameter from CastingConverter - #80755
Merged
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Jan 20, 2023
Merged
Remove the second type parameter from CastingConverter#80755
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Conversation
@eiriktsarpalis
@eiriktsarpaliseiriktsarpalis
commented
Jan 17, 2023•

edited

Loading

Copy link


Copy Markdown

Member
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
@eiriktsarpaliseiriktsarpalis
added this to the 8.0.0 milestone
Jan 17, 2023
@ghost
ghost
commented
Jan 17, 2023

Copy link


Copy Markdown

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
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json
Milestone:8.0.0
eiriktsarpalis
eiriktsarpalis
commented
Jan 17, 2023
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs

 internal abstract bool OnTryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract bool TryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract object? ReadAsObject(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options);
@eiriktsarpaliseiriktsarpalisJan 17, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this change is adding new methods to JsonConverter this should increase code size linearly over the number of generic specializations. This should still contribute to a net decrease of code size given that CastingConverter contributes to quadratic growth over the number of types being used. I will follow up with real numbers validating this hypothesis.
@eerhardt
eerhardt
commented
Jan 17, 2023

Copy link


Copy Markdown

Member
which is contributing to a quadratic explosion of generic specializations in NativeAOT:
Is it possible to get before/after size numbers for this change? If you need a test app, the one we are using in our perf measurements is here.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 18, 2023

Closed

@eiriktsarpalis
eiriktsarpalis
commented
Jan 18, 2023

Copy link


Copy Markdown

MemberAuthor
I compared the publish AOT size for the Goldilocks app between main and the PR branch. I'm seeing a ~7MB improvement in the size of the publish folder, from 120 MB down to 113 MB.
@eiriktsarpalis
eiriktsarpalis
commented
Jan 19, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
STJ Numbers from the mstat dump of the Goldilocks app. Seeing just over 1MB of size reduction overall:
21156 publish-main/Goldilocks
20064 publish-pr/Goldilocks

main
// ********** Types Total Size 3,327,268
System.Text.Json 476,148
// ********** Size By Namespace
System.Text.Json.Serialization.Converters 876,578
System.Text.Json.Serialization.Metadata 607,084
System.Text.Json.Serialization 361,833
System.Text.Json 203,356

PR
// ********** Types Total Size 3,172,132
System.Text.Json 320,884
// ********** Size By Namespace
System.Text.Json.Serialization.Metadata 605,298
System.Text.Json.Serialization 397,892
System.Text.Json 208,571
System.Text.Json.Serialization.Converters 200,477

@eerhardt
eerhardt
commented
Jan 19, 2023

Copy link


Copy Markdown

Member
/benchmark json aspnet-citrine-win libs
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

Benchmark started for json on aspnet-citrine-win with libs. Logs: link
eerhardt
eerhardt
approved these changes
Jan 19, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look over the code (I'm no expert in this area), but I think this is a great change. If there are no runtime perf degradations, I think this is a great NativeAOT size win (5% of the new dotnet new api -aot app).
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

json - aspnet-citrine-win
applicationjson.basejson.pr
CPU Usage (%)8087+8.75%
Cores usage (%)2,2292,433+9.15%
Working Set (MB)7374+1.37%
Private Memory (MB)100101+1.00%
Build Time (ms)4,9793,287-33.98%
Start Time (ms)417418+0.24%
Published Size (KB)96,70296,7020.00%
Symbols Size (KB)52520.00%
.NET Core SDK Version8.0.100-alpha.1.23069.68.0.100-alpha.1.23069.6
loadjson.basejson.pr
CPU Usage (%)7274+2.78%
Cores usage (%)2,0262,084+2.86%
Working Set (MB)1191190.00%
Private Memory (MB)3633630.00%
Start Time (ms)00
First Request (ms)85850.00%
Requests/sec1,059,9091,095,251+3.33%
Requests16,004,21016,537,175+3.33%
Mean latency (ms)0.891.11+24.72%
Max latency (ms)36.8459.44+61.35%
Bad responses00
Socket errors00
Read throughput (MB/s)147.58152.50+3.33%
Latency 50th (ms)0.320.31-4.64%
Latency 75th (ms)0.770.83+8.37%
Latency 90th (ms)2.343.10+32.48%
Latency 99th (ms)7.0210.31+46.87%
krwq
krwq
approved these changes
Jan 20, 2023
View reviewed changes
@krwqkrwq
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome win, thanks!
@krwq
krwq
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
:shipit:
@krwqkrwq
added
the 
size-reduction
Issues impacting final app size primary for size sensitive workloads
label
Jan 20, 2023
@ghost
ghost
commented
Jan 20, 2023

Copy link


Copy Markdown

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar

See info in area-owners.md if you want to be subscribed.
Issue Details
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json, size-reduction
Milestone:8.0.0
@eiriktsarpaliseiriktsarpalis
mentioned this pull request
Jan 20, 2023

Open

@eiriktsarpalis
eiriktsarpalis
merged commit a04aaeb
into
dotnet:mainJan 20, 2023
@eiriktsarpalis
eiriktsarpalis
deleted the

castingconverter-remove-generic-parameter

branch
January 20, 2023 16:15
mdh1418 pushed a commit
to mdh1418/runtime
that referenced
this pull request
Jan 24, 2023
@eiriktsarpalis@mdh1418
Remove second type parameter from CastingConverter (dotnet#80755)
81e99ea
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 19, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@krwqkrwqkrwq approved these changes
@eerhardteerhardteerhardt approved these changes
@layomialayomiaAwaiting requested review from layomia
@tarekghtarekghAwaiting requested review from tarekgh

Assignees

@eiriktsarpaliseiriktsarpalis

Labels

area-System.Text.Jsonsize-reductionIssues impacting final app size primary for size sensitive workloads

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@eiriktsarpalis@eerhardt@krwq

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Remove the second type parameter from CastingConverter by eiriktsarpalis · Pull Request #80755 · dotnet/runtime · GitHub
Skip to content
Remove the second type parameter from CastingConverter - #80755
Merged
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Jan 20, 2023
Merged
Remove the second type parameter from CastingConverter#80755
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Conversation
@eiriktsarpalis
@eiriktsarpaliseiriktsarpalis
commented
Jan 17, 2023•

edited

Loading

Copy link


Copy Markdown

Member
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
@eiriktsarpaliseiriktsarpalis
added this to the 8.0.0 milestone
Jan 17, 2023
@ghost
ghost
commented
Jan 17, 2023

Copy link


Copy Markdown

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
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json
Milestone:8.0.0
eiriktsarpalis
eiriktsarpalis
commented
Jan 17, 2023
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs

 internal abstract bool OnTryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract bool TryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract object? ReadAsObject(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options);
@eiriktsarpaliseiriktsarpalisJan 17, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this change is adding new methods to JsonConverter this should increase code size linearly over the number of generic specializations. This should still contribute to a net decrease of code size given that CastingConverter contributes to quadratic growth over the number of types being used. I will follow up with real numbers validating this hypothesis.
@eerhardt
eerhardt
commented
Jan 17, 2023

Copy link


Copy Markdown

Member
which is contributing to a quadratic explosion of generic specializations in NativeAOT:
Is it possible to get before/after size numbers for this change? If you need a test app, the one we are using in our perf measurements is here.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 18, 2023

Closed

@eiriktsarpalis
eiriktsarpalis
commented
Jan 18, 2023

Copy link


Copy Markdown

MemberAuthor
I compared the publish AOT size for the Goldilocks app between main and the PR branch. I'm seeing a ~7MB improvement in the size of the publish folder, from 120 MB down to 113 MB.
@eiriktsarpalis
eiriktsarpalis
commented
Jan 19, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
STJ Numbers from the mstat dump of the Goldilocks app. Seeing just over 1MB of size reduction overall:
21156 publish-main/Goldilocks
20064 publish-pr/Goldilocks

main
// ********** Types Total Size 3,327,268
System.Text.Json 476,148
// ********** Size By Namespace
System.Text.Json.Serialization.Converters 876,578
System.Text.Json.Serialization.Metadata 607,084
System.Text.Json.Serialization 361,833
System.Text.Json 203,356

PR
// ********** Types Total Size 3,172,132
System.Text.Json 320,884
// ********** Size By Namespace
System.Text.Json.Serialization.Metadata 605,298
System.Text.Json.Serialization 397,892
System.Text.Json 208,571
System.Text.Json.Serialization.Converters 200,477

@eerhardt
eerhardt
commented
Jan 19, 2023

Copy link


Copy Markdown

Member
/benchmark json aspnet-citrine-win libs
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

Benchmark started for json on aspnet-citrine-win with libs. Logs: link
eerhardt
eerhardt
approved these changes
Jan 19, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look over the code (I'm no expert in this area), but I think this is a great change. If there are no runtime perf degradations, I think this is a great NativeAOT size win (5% of the new dotnet new api -aot app).
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

json - aspnet-citrine-win
applicationjson.basejson.pr
CPU Usage (%)8087+8.75%
Cores usage (%)2,2292,433+9.15%
Working Set (MB)7374+1.37%
Private Memory (MB)100101+1.00%
Build Time (ms)4,9793,287-33.98%
Start Time (ms)417418+0.24%
Published Size (KB)96,70296,7020.00%
Symbols Size (KB)52520.00%
.NET Core SDK Version8.0.100-alpha.1.23069.68.0.100-alpha.1.23069.6
loadjson.basejson.pr
CPU Usage (%)7274+2.78%
Cores usage (%)2,0262,084+2.86%
Working Set (MB)1191190.00%
Private Memory (MB)3633630.00%
Start Time (ms)00
First Request (ms)85850.00%
Requests/sec1,059,9091,095,251+3.33%
Requests16,004,21016,537,175+3.33%
Mean latency (ms)0.891.11+24.72%
Max latency (ms)36.8459.44+61.35%
Bad responses00
Socket errors00
Read throughput (MB/s)147.58152.50+3.33%
Latency 50th (ms)0.320.31-4.64%
Latency 75th (ms)0.770.83+8.37%
Latency 90th (ms)2.343.10+32.48%
Latency 99th (ms)7.0210.31+46.87%
krwq
krwq
approved these changes
Jan 20, 2023
View reviewed changes
@krwqkrwq
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome win, thanks!
@krwq
krwq
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
:shipit:
@krwqkrwq
added
the 
size-reduction
Issues impacting final app size primary for size sensitive workloads
label
Jan 20, 2023
@ghost
ghost
commented
Jan 20, 2023

Copy link


Copy Markdown

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar

See info in area-owners.md if you want to be subscribed.
Issue Details
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json, size-reduction
Milestone:8.0.0
@eiriktsarpaliseiriktsarpalis
mentioned this pull request
Jan 20, 2023

Open

@eiriktsarpalis
eiriktsarpalis
merged commit a04aaeb
into
dotnet:mainJan 20, 2023
@eiriktsarpalis
eiriktsarpalis
deleted the

castingconverter-remove-generic-parameter

branch
January 20, 2023 16:15
mdh1418 pushed a commit
to mdh1418/runtime
that referenced
this pull request
Jan 24, 2023
@eiriktsarpalis@mdh1418
Remove second type parameter from CastingConverter (dotnet#80755)
81e99ea
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 19, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@krwqkrwqkrwq approved these changes
@eerhardteerhardteerhardt approved these changes
@layomialayomiaAwaiting requested review from layomia
@tarekghtarekghAwaiting requested review from tarekgh

Assignees

@eiriktsarpaliseiriktsarpalis

Labels

area-System.Text.Jsonsize-reductionIssues impacting final app size primary for size sensitive workloads

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@eiriktsarpalis@eerhardt@krwq

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
Remove the second type parameter from CastingConverter by eiriktsarpalis · Pull Request #80755 · dotnet/runtime · GitHub
Skip to content
Remove the second type parameter from CastingConverter - #80755
Merged
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Jan 20, 2023
Merged
Remove the second type parameter from CastingConverter#80755
eiriktsarpalis merged 1 commit into
dotnet:mainfrom 
eiriktsarpalis:castingconverter-remove-generic-parameter
Conversation
@eiriktsarpalis
@eiriktsarpaliseiriktsarpalis
commented
Jan 17, 2023•

edited

Loading

Copy link


Copy Markdown

Member
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
@eiriktsarpaliseiriktsarpalis
added this to the 8.0.0 milestone
Jan 17, 2023
@ghost
ghost
commented
Jan 17, 2023

Copy link


Copy Markdown

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
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json
Milestone:8.0.0
eiriktsarpalis
eiriktsarpalis
commented
Jan 17, 2023
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs

 internal abstract bool OnTryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract bool TryReadAsObject(ref Utf8JsonReader reader, JsonSerializerOptions options, scoped ref ReadStack state, out object? value);
 internal abstract object? ReadAsObject(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options);
@eiriktsarpaliseiriktsarpalisJan 17, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this change is adding new methods to JsonConverter this should increase code size linearly over the number of generic specializations. This should still contribute to a net decrease of code size given that CastingConverter contributes to quadratic growth over the number of types being used. I will follow up with real numbers validating this hypothesis.
@eerhardt
eerhardt
commented
Jan 17, 2023

Copy link


Copy Markdown

Member
which is contributing to a quadratic explosion of generic specializations in NativeAOT:
Is it possible to get before/after size numbers for this change? If you need a test app, the one we are using in our perf measurements is here.
@build-analysisbuild-analysisBot
mentioned this pull request
Jan 18, 2023

Closed

@eiriktsarpalis
eiriktsarpalis
commented
Jan 18, 2023

Copy link


Copy Markdown

MemberAuthor
I compared the publish AOT size for the Goldilocks app between main and the PR branch. I'm seeing a ~7MB improvement in the size of the publish folder, from 120 MB down to 113 MB.
@eiriktsarpalis
eiriktsarpalis
commented
Jan 19, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
STJ Numbers from the mstat dump of the Goldilocks app. Seeing just over 1MB of size reduction overall:
21156 publish-main/Goldilocks
20064 publish-pr/Goldilocks

main
// ********** Types Total Size 3,327,268
System.Text.Json 476,148
// ********** Size By Namespace
System.Text.Json.Serialization.Converters 876,578
System.Text.Json.Serialization.Metadata 607,084
System.Text.Json.Serialization 361,833
System.Text.Json 203,356

PR
// ********** Types Total Size 3,172,132
System.Text.Json 320,884
// ********** Size By Namespace
System.Text.Json.Serialization.Metadata 605,298
System.Text.Json.Serialization 397,892
System.Text.Json 208,571
System.Text.Json.Serialization.Converters 200,477

@eerhardt
eerhardt
commented
Jan 19, 2023

Copy link


Copy Markdown

Member
/benchmark json aspnet-citrine-win libs
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

Benchmark started for json on aspnet-citrine-win with libs. Logs: link
eerhardt
eerhardt
approved these changes
Jan 19, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look over the code (I'm no expert in this area), but I think this is a great change. If there are no runtime perf degradations, I think this is a great NativeAOT size win (5% of the new dotnet new api -aot app).
@pr-benchmarks
pr-benchmarksBot
commented
Jan 19, 2023

Copy link


Copy Markdown

json - aspnet-citrine-win
applicationjson.basejson.pr
CPU Usage (%)8087+8.75%
Cores usage (%)2,2292,433+9.15%
Working Set (MB)7374+1.37%
Private Memory (MB)100101+1.00%
Build Time (ms)4,9793,287-33.98%
Start Time (ms)417418+0.24%
Published Size (KB)96,70296,7020.00%
Symbols Size (KB)52520.00%
.NET Core SDK Version8.0.100-alpha.1.23069.68.0.100-alpha.1.23069.6
loadjson.basejson.pr
CPU Usage (%)7274+2.78%
Cores usage (%)2,0262,084+2.86%
Working Set (MB)1191190.00%
Private Memory (MB)3633630.00%
Start Time (ms)00
First Request (ms)85850.00%
Requests/sec1,059,9091,095,251+3.33%
Requests16,004,21016,537,175+3.33%
Mean latency (ms)0.891.11+24.72%
Max latency (ms)36.8459.44+61.35%
Bad responses00
Socket errors00
Read throughput (MB/s)147.58152.50+3.33%
Latency 50th (ms)0.320.31-4.64%
Latency 75th (ms)0.770.83+8.37%
Latency 90th (ms)2.343.10+32.48%
Latency 99th (ms)7.0210.31+46.87%
krwq
krwq
approved these changes
Jan 20, 2023
View reviewed changes
@krwqkrwq
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome win, thanks!
@krwq
krwq
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
:shipit:
@krwqkrwq
added
the 
size-reduction
Issues impacting final app size primary for size sensitive workloads
label
Jan 20, 2023
@ghost
ghost
commented
Jan 20, 2023

Copy link


Copy Markdown

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar

See info in area-owners.md if you want to be subscribed.
Issue Details
The CastingConverter<TSource, TTarget> class is an internal adapter used for custom converters whose declared type doesn't match that of the serialized member they are being assigned to. The problem with the current implementation is that CastingConverter uses two type parameters, which is contributing to a quadratic explosion of generic specializations in NativeAOT:
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Text.Json.JsonElement>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Boolean>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Byte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Char>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTime>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.DateTimeOffset>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Decimal>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Double>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Guid>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Int64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.SByte>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Single>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeOnly>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.TimeSpan>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt16>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt32>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.UInt64>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.__Canon,System.__Canon>. 384
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Text.Json.JsonElement,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Boolean,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Byte,System.__Canon>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>. 376
System.Text.Json.Serialization.Converters.CastingConverter`2<System.Char,System.__Canon>. 
This PR relaxes the implementation so that CastingConverter references and consumes the underlying converter as a weakly typed JsonConverter instance. This change can regress runtime performance in certain scenaria due to added boxing/virtual calls, but this should only impact members that are using converters polymorphically -- a relatively rare occurrence.
cc @eerhardt@jkotas
Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:
area-System.Text.Json, size-reduction
Milestone:8.0.0
@eiriktsarpaliseiriktsarpalis
mentioned this pull request
Jan 20, 2023

Open

@eiriktsarpalis
eiriktsarpalis
merged commit a04aaeb
into
dotnet:mainJan 20, 2023
@eiriktsarpalis
eiriktsarpalis
deleted the

castingconverter-remove-generic-parameter

branch
January 20, 2023 16:15
mdh1418 pushed a commit
to mdh1418/runtime
that referenced
this pull request
Jan 24, 2023
@eiriktsarpalis@mdh1418
Remove second type parameter from CastingConverter (dotnet#80755)
81e99ea
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 19, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@krwqkrwqkrwq approved these changes
@eerhardteerhardteerhardt approved these changes
@layomialayomiaAwaiting requested review from layomia
@tarekghtarekghAwaiting requested review from tarekgh

Assignees

@eiriktsarpaliseiriktsarpalis

Labels

area-System.Text.Jsonsize-reductionIssues impacting final app size primary for size sensitive workloads

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@eiriktsarpalis@eerhardt@krwq