Avoid boxing in System.ObjectModel.KeydCollection during startup - #104504

Closed
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing
Closed

Avoid boxing in System.ObjectModel.KeydCollection during startup#104504
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing

Conversation

@jkotas

Copy link
Copy Markdown
Member

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup. Switch KeyedCollection to use ThrowHelper that is pattern used by number of other collections.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2024
@jkotas

Copy link
Copy Markdown
MemberAuthor

This was noted in #103361 (comment) .

@jkotasjkotas changed the title Avoid boxing System.ObjectModel during startupAvoid boxing in System.ObjectModel.KeydCollection during startupJul 6, 2024
ArgumentNullException.ThrowIfNull can incur boxing when applied to argument
of generic type. Tier-1 JIT optimizations are able to optimize this boxing
in steady state, but Tier-0 JIT optimization are not. It can result into excessive
allocations during startup. Switch KeyedCollection to use ThrowHelper
that is pattern used by number of other collections.
@jkotas

Copy link
Copy Markdown
MemberAuthor

Repro:

using System.Collections.ObjectModel;
var c = new MyCollection();
var start = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 1000; i++)
{
c.TryGetValue(1, out _);
}
var end = GC.GetAllocatedBytesForCurrentThread();
Console.WriteLine(end - start);
class MyCollection : KeyedCollection<uint, uint>
{
protected override uint GetKeyForItem(uint item) => item;
}

24000 before the change, 0 with the change.

@jkotasjkotas added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past, and we've previously defended the non-generic ThrowIfNull for such use, citing the boxing removal by the JIT. If we're going to start replacing these, I think we should instead reconsider adding such a generic overload, or augment the JIT to do the boxing removal even in tier 0.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past

I was not aware that we had issues opened on this in the past. For reference: #82227 and #100406

On the other hand, we took tweaks to avoid Tier-0 specific boxing in the past.

we should instead reconsider adding such a generic overload

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

augment the JIT to do the boxing removal even in tier 0.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

@AndyAyersMS

Copy link
Copy Markdown
Member

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

@333fred

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

To be clear, you'd want public static void ThrowIfNull<T>(T? argument, string? paramName = default), but for that to only be prioritized when T is itself a type parameter? No, that isn't possible to do with priorities as designed. Priority is applied unconditionally.

@jkotas

Copy link
Copy Markdown
MemberAuthor

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

That sounds better to me than applying workarounds like this in generic collections and other types. Opened #104512

@jkotasjkotas closed this Jul 6, 2024
@jkotas
jkotas deleted the boxing branch July 6, 2024 19:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@stephentoub@AndyAyersMS@333fred@eiriktsarpalis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Avoid boxing in System.ObjectModel.KeydCollection during startup - #104504

Closed
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing
Closed

Avoid boxing in System.ObjectModel.KeydCollection during startup#104504
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing

Conversation

@jkotas

Copy link
Copy Markdown
Member

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup. Switch KeyedCollection to use ThrowHelper that is pattern used by number of other collections.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2024
@jkotas

Copy link
Copy Markdown
MemberAuthor

This was noted in #103361 (comment) .

@jkotasjkotas changed the title Avoid boxing System.ObjectModel during startupAvoid boxing in System.ObjectModel.KeydCollection during startupJul 6, 2024
ArgumentNullException.ThrowIfNull can incur boxing when applied to argument
of generic type. Tier-1 JIT optimizations are able to optimize this boxing
in steady state, but Tier-0 JIT optimization are not. It can result into excessive
allocations during startup. Switch KeyedCollection to use ThrowHelper
that is pattern used by number of other collections.
@jkotas

Copy link
Copy Markdown
MemberAuthor

Repro:

using System.Collections.ObjectModel;
var c = new MyCollection();
var start = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 1000; i++)
{
c.TryGetValue(1, out _);
}
var end = GC.GetAllocatedBytesForCurrentThread();
Console.WriteLine(end - start);
class MyCollection : KeyedCollection<uint, uint>
{
protected override uint GetKeyForItem(uint item) => item;
}

24000 before the change, 0 with the change.

@jkotasjkotas added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past, and we've previously defended the non-generic ThrowIfNull for such use, citing the boxing removal by the JIT. If we're going to start replacing these, I think we should instead reconsider adding such a generic overload, or augment the JIT to do the boxing removal even in tier 0.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past

I was not aware that we had issues opened on this in the past. For reference: #82227 and #100406

On the other hand, we took tweaks to avoid Tier-0 specific boxing in the past.

we should instead reconsider adding such a generic overload

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

augment the JIT to do the boxing removal even in tier 0.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

@AndyAyersMS

Copy link
Copy Markdown
Member

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

@333fred

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

To be clear, you'd want public static void ThrowIfNull<T>(T? argument, string? paramName = default), but for that to only be prioritized when T is itself a type parameter? No, that isn't possible to do with priorities as designed. Priority is applied unconditionally.

@jkotas

Copy link
Copy Markdown
MemberAuthor

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

That sounds better to me than applying workarounds like this in generic collections and other types. Opened #104512

@jkotasjkotas closed this Jul 6, 2024
@jkotas
jkotas deleted the boxing branch July 6, 2024 19:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@stephentoub@AndyAyersMS@333fred@eiriktsarpalis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Avoid boxing in System.ObjectModel.KeydCollection during startup - #104504

Closed
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing
Closed

Avoid boxing in System.ObjectModel.KeydCollection during startup#104504
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing

Conversation

@jkotas

Copy link
Copy Markdown
Member

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup. Switch KeyedCollection to use ThrowHelper that is pattern used by number of other collections.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2024
@jkotas

Copy link
Copy Markdown
MemberAuthor

This was noted in #103361 (comment) .

@jkotasjkotas changed the title Avoid boxing System.ObjectModel during startupAvoid boxing in System.ObjectModel.KeydCollection during startupJul 6, 2024
ArgumentNullException.ThrowIfNull can incur boxing when applied to argument
of generic type. Tier-1 JIT optimizations are able to optimize this boxing
in steady state, but Tier-0 JIT optimization are not. It can result into excessive
allocations during startup. Switch KeyedCollection to use ThrowHelper
that is pattern used by number of other collections.
@jkotas

Copy link
Copy Markdown
MemberAuthor

Repro:

using System.Collections.ObjectModel;
var c = new MyCollection();
var start = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 1000; i++)
{
c.TryGetValue(1, out _);
}
var end = GC.GetAllocatedBytesForCurrentThread();
Console.WriteLine(end - start);
class MyCollection : KeyedCollection<uint, uint>
{
protected override uint GetKeyForItem(uint item) => item;
}

24000 before the change, 0 with the change.

@jkotasjkotas added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past, and we've previously defended the non-generic ThrowIfNull for such use, citing the boxing removal by the JIT. If we're going to start replacing these, I think we should instead reconsider adding such a generic overload, or augment the JIT to do the boxing removal even in tier 0.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past

I was not aware that we had issues opened on this in the past. For reference: #82227 and #100406

On the other hand, we took tweaks to avoid Tier-0 specific boxing in the past.

we should instead reconsider adding such a generic overload

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

augment the JIT to do the boxing removal even in tier 0.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

@AndyAyersMS

Copy link
Copy Markdown
Member

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

@333fred

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

To be clear, you'd want public static void ThrowIfNull<T>(T? argument, string? paramName = default), but for that to only be prioritized when T is itself a type parameter? No, that isn't possible to do with priorities as designed. Priority is applied unconditionally.

@jkotas

Copy link
Copy Markdown
MemberAuthor

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

That sounds better to me than applying workarounds like this in generic collections and other types. Opened #104512

@jkotasjkotas closed this Jul 6, 2024
@jkotas
jkotas deleted the boxing branch July 6, 2024 19:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@stephentoub@AndyAyersMS@333fred@eiriktsarpalis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Avoid boxing in System.ObjectModel.KeydCollection during startup - #104504

Closed
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing
Closed

Avoid boxing in System.ObjectModel.KeydCollection during startup#104504
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing

Conversation

@jkotas

Copy link
Copy Markdown
Member

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup. Switch KeyedCollection to use ThrowHelper that is pattern used by number of other collections.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2024
@jkotas

Copy link
Copy Markdown
MemberAuthor

This was noted in #103361 (comment) .

@jkotasjkotas changed the title Avoid boxing System.ObjectModel during startupAvoid boxing in System.ObjectModel.KeydCollection during startupJul 6, 2024
ArgumentNullException.ThrowIfNull can incur boxing when applied to argument
of generic type. Tier-1 JIT optimizations are able to optimize this boxing
in steady state, but Tier-0 JIT optimization are not. It can result into excessive
allocations during startup. Switch KeyedCollection to use ThrowHelper
that is pattern used by number of other collections.
@jkotas

Copy link
Copy Markdown
MemberAuthor

Repro:

using System.Collections.ObjectModel;
var c = new MyCollection();
var start = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 1000; i++)
{
c.TryGetValue(1, out _);
}
var end = GC.GetAllocatedBytesForCurrentThread();
Console.WriteLine(end - start);
class MyCollection : KeyedCollection<uint, uint>
{
protected override uint GetKeyForItem(uint item) => item;
}

24000 before the change, 0 with the change.

@jkotasjkotas added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past, and we've previously defended the non-generic ThrowIfNull for such use, citing the boxing removal by the JIT. If we're going to start replacing these, I think we should instead reconsider adding such a generic overload, or augment the JIT to do the boxing removal even in tier 0.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past

I was not aware that we had issues opened on this in the past. For reference: #82227 and #100406

On the other hand, we took tweaks to avoid Tier-0 specific boxing in the past.

we should instead reconsider adding such a generic overload

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

augment the JIT to do the boxing removal even in tier 0.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

@AndyAyersMS

Copy link
Copy Markdown
Member

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

@333fred

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

To be clear, you'd want public static void ThrowIfNull<T>(T? argument, string? paramName = default), but for that to only be prioritized when T is itself a type parameter? No, that isn't possible to do with priorities as designed. Priority is applied unconditionally.

@jkotas

Copy link
Copy Markdown
MemberAuthor

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

That sounds better to me than applying workarounds like this in generic collections and other types. Opened #104512

@jkotasjkotas closed this Jul 6, 2024
@jkotas
jkotas deleted the boxing branch July 6, 2024 19:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@stephentoub@AndyAyersMS@333fred@eiriktsarpalis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Avoid boxing in System.ObjectModel.KeydCollection during startup - #104504

Closed
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing
Closed

Avoid boxing in System.ObjectModel.KeydCollection during startup#104504
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing

Conversation

@jkotas

Copy link
Copy Markdown
Member

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup. Switch KeyedCollection to use ThrowHelper that is pattern used by number of other collections.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2024
@jkotas

Copy link
Copy Markdown
MemberAuthor

This was noted in #103361 (comment) .

@jkotasjkotas changed the title Avoid boxing System.ObjectModel during startupAvoid boxing in System.ObjectModel.KeydCollection during startupJul 6, 2024
ArgumentNullException.ThrowIfNull can incur boxing when applied to argument
of generic type. Tier-1 JIT optimizations are able to optimize this boxing
in steady state, but Tier-0 JIT optimization are not. It can result into excessive
allocations during startup. Switch KeyedCollection to use ThrowHelper
that is pattern used by number of other collections.
@jkotas

Copy link
Copy Markdown
MemberAuthor

Repro:

using System.Collections.ObjectModel;
var c = new MyCollection();
var start = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 1000; i++)
{
c.TryGetValue(1, out _);
}
var end = GC.GetAllocatedBytesForCurrentThread();
Console.WriteLine(end - start);
class MyCollection : KeyedCollection<uint, uint>
{
protected override uint GetKeyForItem(uint item) => item;
}

24000 before the change, 0 with the change.

@jkotasjkotas added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past, and we've previously defended the non-generic ThrowIfNull for such use, citing the boxing removal by the JIT. If we're going to start replacing these, I think we should instead reconsider adding such a generic overload, or augment the JIT to do the boxing removal even in tier 0.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past

I was not aware that we had issues opened on this in the past. For reference: #82227 and #100406

On the other hand, we took tweaks to avoid Tier-0 specific boxing in the past.

we should instead reconsider adding such a generic overload

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

augment the JIT to do the boxing removal even in tier 0.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

@AndyAyersMS

Copy link
Copy Markdown
Member

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

@333fred

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

To be clear, you'd want public static void ThrowIfNull<T>(T? argument, string? paramName = default), but for that to only be prioritized when T is itself a type parameter? No, that isn't possible to do with priorities as designed. Priority is applied unconditionally.

@jkotas

Copy link
Copy Markdown
MemberAuthor

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

That sounds better to me than applying workarounds like this in generic collections and other types. Opened #104512

@jkotasjkotas closed this Jul 6, 2024
@jkotas
jkotas deleted the boxing branch July 6, 2024 19:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@stephentoub@AndyAyersMS@333fred@eiriktsarpalis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Avoid boxing in System.ObjectModel.KeydCollection during startup - #104504

Closed
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing
Closed

Avoid boxing in System.ObjectModel.KeydCollection during startup#104504
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing

Conversation

@jkotas

Copy link
Copy Markdown
Member

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup. Switch KeyedCollection to use ThrowHelper that is pattern used by number of other collections.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2024
@jkotas

Copy link
Copy Markdown
MemberAuthor

This was noted in #103361 (comment) .

@jkotasjkotas changed the title Avoid boxing System.ObjectModel during startupAvoid boxing in System.ObjectModel.KeydCollection during startupJul 6, 2024
ArgumentNullException.ThrowIfNull can incur boxing when applied to argument
of generic type. Tier-1 JIT optimizations are able to optimize this boxing
in steady state, but Tier-0 JIT optimization are not. It can result into excessive
allocations during startup. Switch KeyedCollection to use ThrowHelper
that is pattern used by number of other collections.
@jkotas

Copy link
Copy Markdown
MemberAuthor

Repro:

using System.Collections.ObjectModel;
var c = new MyCollection();
var start = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 1000; i++)
{
c.TryGetValue(1, out _);
}
var end = GC.GetAllocatedBytesForCurrentThread();
Console.WriteLine(end - start);
class MyCollection : KeyedCollection<uint, uint>
{
protected override uint GetKeyForItem(uint item) => item;
}

24000 before the change, 0 with the change.

@jkotasjkotas added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past, and we've previously defended the non-generic ThrowIfNull for such use, citing the boxing removal by the JIT. If we're going to start replacing these, I think we should instead reconsider adding such a generic overload, or augment the JIT to do the boxing removal even in tier 0.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past

I was not aware that we had issues opened on this in the past. For reference: #82227 and #100406

On the other hand, we took tweaks to avoid Tier-0 specific boxing in the past.

we should instead reconsider adding such a generic overload

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

augment the JIT to do the boxing removal even in tier 0.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

@AndyAyersMS

Copy link
Copy Markdown
Member

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

@333fred

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

To be clear, you'd want public static void ThrowIfNull<T>(T? argument, string? paramName = default), but for that to only be prioritized when T is itself a type parameter? No, that isn't possible to do with priorities as designed. Priority is applied unconditionally.

@jkotas

Copy link
Copy Markdown
MemberAuthor

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

That sounds better to me than applying workarounds like this in generic collections and other types. Opened #104512

@jkotasjkotas closed this Jul 6, 2024
@jkotas
jkotas deleted the boxing branch July 6, 2024 19:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@stephentoub@AndyAyersMS@333fred@eiriktsarpalis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Avoid boxing in System.ObjectModel.KeydCollection during startup - #104504

Closed
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing
Closed

Avoid boxing in System.ObjectModel.KeydCollection during startup#104504
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing

Conversation

@jkotas

Copy link
Copy Markdown
Member

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup. Switch KeyedCollection to use ThrowHelper that is pattern used by number of other collections.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2024
@jkotas

Copy link
Copy Markdown
MemberAuthor

This was noted in #103361 (comment) .

@jkotasjkotas changed the title Avoid boxing System.ObjectModel during startupAvoid boxing in System.ObjectModel.KeydCollection during startupJul 6, 2024
ArgumentNullException.ThrowIfNull can incur boxing when applied to argument
of generic type. Tier-1 JIT optimizations are able to optimize this boxing
in steady state, but Tier-0 JIT optimization are not. It can result into excessive
allocations during startup. Switch KeyedCollection to use ThrowHelper
that is pattern used by number of other collections.
@jkotas

Copy link
Copy Markdown
MemberAuthor

Repro:

using System.Collections.ObjectModel;
var c = new MyCollection();
var start = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 1000; i++)
{
c.TryGetValue(1, out _);
}
var end = GC.GetAllocatedBytesForCurrentThread();
Console.WriteLine(end - start);
class MyCollection : KeyedCollection<uint, uint>
{
protected override uint GetKeyForItem(uint item) => item;
}

24000 before the change, 0 with the change.

@jkotasjkotas added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past, and we've previously defended the non-generic ThrowIfNull for such use, citing the boxing removal by the JIT. If we're going to start replacing these, I think we should instead reconsider adding such a generic overload, or augment the JIT to do the boxing removal even in tier 0.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past

I was not aware that we had issues opened on this in the past. For reference: #82227 and #100406

On the other hand, we took tweaks to avoid Tier-0 specific boxing in the past.

we should instead reconsider adding such a generic overload

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

augment the JIT to do the boxing removal even in tier 0.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

@AndyAyersMS

Copy link
Copy Markdown
Member

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

@333fred

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

To be clear, you'd want public static void ThrowIfNull<T>(T? argument, string? paramName = default), but for that to only be prioritized when T is itself a type parameter? No, that isn't possible to do with priorities as designed. Priority is applied unconditionally.

@jkotas

Copy link
Copy Markdown
MemberAuthor

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

That sounds better to me than applying workarounds like this in generic collections and other types. Opened #104512

@jkotasjkotas closed this Jul 6, 2024
@jkotas
jkotas deleted the boxing branch July 6, 2024 19:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@stephentoub@AndyAyersMS@333fred@eiriktsarpalis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Avoid boxing in System.ObjectModel.KeydCollection during startup - #104504

Closed
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing
Closed

Avoid boxing in System.ObjectModel.KeydCollection during startup#104504
jkotas wants to merge 1 commit into
dotnet:mainfrom
jkotas:boxing

Conversation

@jkotas

Copy link
Copy Markdown
Member

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup. Switch KeyedCollection to use ThrowHelper that is pattern used by number of other collections.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2024
@jkotas

Copy link
Copy Markdown
MemberAuthor

This was noted in #103361 (comment) .

@jkotasjkotas changed the title Avoid boxing System.ObjectModel during startupAvoid boxing in System.ObjectModel.KeydCollection during startupJul 6, 2024
ArgumentNullException.ThrowIfNull can incur boxing when applied to argument
of generic type. Tier-1 JIT optimizations are able to optimize this boxing
in steady state, but Tier-0 JIT optimization are not. It can result into excessive
allocations during startup. Switch KeyedCollection to use ThrowHelper
that is pattern used by number of other collections.
@jkotas

Copy link
Copy Markdown
MemberAuthor

Repro:

using System.Collections.ObjectModel;
var c = new MyCollection();
var start = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 1000; i++)
{
c.TryGetValue(1, out _);
}
var end = GC.GetAllocatedBytesForCurrentThread();
Console.WriteLine(end - start);
class MyCollection : KeyedCollection<uint, uint>
{
protected override uint GetKeyForItem(uint item) => item;
}

24000 before the change, 0 with the change.

@jkotasjkotas added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past, and we've previously defended the non-generic ThrowIfNull for such use, citing the boxing removal by the JIT. If we're going to start replacing these, I think we should instead reconsider adding such a generic overload, or augment the JIT to do the boxing removal even in tier 0.

@jkotas

Copy link
Copy Markdown
MemberAuthor

Issues on this (not just for KeyedCollection, but multiple places ThrowIfNull is used with a generic) have been opened multiple times in the past

I was not aware that we had issues opened on this in the past. For reference: #82227 and #100406

On the other hand, we took tweaks to avoid Tier-0 specific boxing in the past.

we should instead reconsider adding such a generic overload

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

augment the JIT to do the boxing removal even in tier 0.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

@stephentoub

stephentoub commented Jul 6, 2024

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

@AndyAyersMS

Copy link
Copy Markdown
Member

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

@333fred

Copy link
Copy Markdown
Member

Is there a way to make this generic overload bind to generic types only?

Not to my knowledge. @333fred, any way to use overload priorities to achieve this?

To be clear, you'd want public static void ThrowIfNull<T>(T? argument, string? paramName = default), but for that to only be prioritized when T is itself a type parameter? No, that isn't possible to do with priorities as designed. Priority is applied unconditionally.

@jkotas

Copy link
Copy Markdown
MemberAuthor

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

That sounds better to me than applying workarounds like this in generic collections and other types. Opened #104512

@jkotasjkotas closed this Jul 6, 2024
@jkotas
jkotas deleted the boxing branch July 6, 2024 19:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@stephentoub@AndyAyersMS@333fred@eiriktsarpalis