Reduce allocations in Quic/HTTP3 - #104394

Merged
MihaZupan merged 2 commits into
dotnet:mainfrom
MihaZupan:quic-allocs
Jul 8, 2024
Merged

Reduce allocations in Quic/HTTP3#104394
MihaZupan merged 2 commits into
dotnet:mainfrom
MihaZupan:quic-allocs

Conversation

@MihaZupan

@MihaZupanMihaZupan commented Jul 3, 2024

Copy link
Copy Markdown
Member

As of #104035 we're now waiting for WritesClosed on every request, and paying for the allocations backing that Task. This PR attempts to claw that perf back.

This does a couple of things:

  • if the ResettableValueTaskSource's final task was already signaled by the time someone first asked for the Task, avoid allocating the intermediate TaskCompletionSource and its Task.
  • Move the waiting on WritesClosed later on in Http3's SendAsync (after receiving the response) to make it more likely that WritesClosed will have been completed by the time we check, thus avoiding the allocations.
  • Honors the cancellationToken while waiting for WritesClosed -- @ManickaP I'm guessing that's desirable here?
  • Avoids the state machine allocation in FlushSendBufferAsync

@MihaZupanMihaZupan added this to the 9.0.0 milestone Jul 3, 2024
@MihaZupan
MihaZupan requested review from a team and ManickaPJuly 3, 2024 18:23
@MihaZupanMihaZupan self-assigned this Jul 3, 2024
@dotnet-policy-service

dotnet-policy-serviceBot commented Jul 3, 2024

Copy link
Copy Markdown
Contributor

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

@MihaZupan

MihaZupan commented Jul 3, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Jul 3, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

rzikm
rzikm approved these changes Jul 4, 2024
View reviewed changes

@rzikmrzikm 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.

LGTM, thanks!

ManickaP
ManickaP reviewed Jul 4, 2024
View reviewed changes

@ManickaPManickaP 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.

Before this PR, WritesClosed was handled right after completing writes, logically next to each other. Now, the logic is split in different places. I know it's more efficient, but IMHO it's much harder to follow now and not mess up in the future.

I'll not block this PR on this, I'm just sharing my perspective.

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs Outdated
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs Outdated
This was referenced Jul 5, 2024
Open
Closed
Closed
@MihaZupan

MihaZupan commented Jul 6, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Jul 6, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
MihaZupan merged commit 2d92034 into dotnet:mainJul 8, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 9, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ManickaPManickaPManickaP approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Quic

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MihaZupan@ManickaP@rzikm
, '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
Reduce allocations in Quic/HTTP3 - #104394
Merged
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Jul 8, 2024
Merged
Reduce allocations in Quic/HTTP3#104394
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Conversation
@MihaZupan
@MihaZupanMihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

Member
As of #104035 we're now waiting for WritesClosed on every request, and paying for the allocations backing that Task. This PR attempts to claw that perf back.
This does a couple of things:
  • if the ResettableValueTaskSource's final task was already signaled by the time someone first asked for the Task, avoid allocating the intermediate TaskCompletionSource and its Task.
  • Move the waiting on WritesClosed later on in Http3's SendAsync (after receiving the response) to make it more likely that WritesClosed will have been completed by the time we check, thus avoiding the allocations.
  • Honors the cancellationToken while waiting for WritesClosed -- @ManickaP I'm guessing that's desirable here?
  • Avoids the state machine allocation in FlushSendBufferAsync
@MihaZupanMihaZupan
added this to the 9.0.0 milestone
Jul 3, 2024
@MihaZupan
MihaZupan
requested review from
a team and ManickaPJuly 3, 2024 18:23
@MihaZupanMihaZupan
self-assigned this
Jul 3, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Jul 3, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 3, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
rzikm
rzikm
approved these changes
Jul 4, 2024
View reviewed changes
@rzikmrzikm
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.

LGTM, thanks!
ManickaP
ManickaP
reviewed
Jul 4, 2024
View reviewed changes
@ManickaPManickaP
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.

Before this PR, WritesClosed was handled right after completing writes, logically next to each other. Now, the logic is split in different places. I know it's more efficient, but IMHO it's much harder to follow now and not mess up in the future.
I'll not block this PR on this, I'm just sharing my perspective.
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated


This was referenced Jul 5, 2024

Open


Closed


Closed

@MihaZupan
MihaZupan
commented
Jul 6, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 6, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 2d92034
into
dotnet:mainJul 8, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 9, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@ManickaPManickaPManickaP approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Quic

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MihaZupan@ManickaP@rzikm

, '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
Reduce allocations in Quic/HTTP3 - #104394
Merged
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Jul 8, 2024
Merged
Reduce allocations in Quic/HTTP3#104394
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Conversation
@MihaZupan
@MihaZupanMihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

Member
As of #104035 we're now waiting for WritesClosed on every request, and paying for the allocations backing that Task. This PR attempts to claw that perf back.
This does a couple of things:
  • if the ResettableValueTaskSource's final task was already signaled by the time someone first asked for the Task, avoid allocating the intermediate TaskCompletionSource and its Task.
  • Move the waiting on WritesClosed later on in Http3's SendAsync (after receiving the response) to make it more likely that WritesClosed will have been completed by the time we check, thus avoiding the allocations.
  • Honors the cancellationToken while waiting for WritesClosed -- @ManickaP I'm guessing that's desirable here?
  • Avoids the state machine allocation in FlushSendBufferAsync
@MihaZupanMihaZupan
added this to the 9.0.0 milestone
Jul 3, 2024
@MihaZupan
MihaZupan
requested review from
a team and ManickaPJuly 3, 2024 18:23
@MihaZupanMihaZupan
self-assigned this
Jul 3, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Jul 3, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 3, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
rzikm
rzikm
approved these changes
Jul 4, 2024
View reviewed changes
@rzikmrzikm
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.

LGTM, thanks!
ManickaP
ManickaP
reviewed
Jul 4, 2024
View reviewed changes
@ManickaPManickaP
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.

Before this PR, WritesClosed was handled right after completing writes, logically next to each other. Now, the logic is split in different places. I know it's more efficient, but IMHO it's much harder to follow now and not mess up in the future.
I'll not block this PR on this, I'm just sharing my perspective.
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated


This was referenced Jul 5, 2024

Open


Closed


Closed

@MihaZupan
MihaZupan
commented
Jul 6, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 6, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 2d92034
into
dotnet:mainJul 8, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 9, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@ManickaPManickaPManickaP approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Quic

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MihaZupan@ManickaP@rzikm

, '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
Reduce allocations in Quic/HTTP3 - #104394
Merged
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Jul 8, 2024
Merged
Reduce allocations in Quic/HTTP3#104394
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Conversation
@MihaZupan
@MihaZupanMihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

Member
As of #104035 we're now waiting for WritesClosed on every request, and paying for the allocations backing that Task. This PR attempts to claw that perf back.
This does a couple of things:
  • if the ResettableValueTaskSource's final task was already signaled by the time someone first asked for the Task, avoid allocating the intermediate TaskCompletionSource and its Task.
  • Move the waiting on WritesClosed later on in Http3's SendAsync (after receiving the response) to make it more likely that WritesClosed will have been completed by the time we check, thus avoiding the allocations.
  • Honors the cancellationToken while waiting for WritesClosed -- @ManickaP I'm guessing that's desirable here?
  • Avoids the state machine allocation in FlushSendBufferAsync
@MihaZupanMihaZupan
added this to the 9.0.0 milestone
Jul 3, 2024
@MihaZupan
MihaZupan
requested review from
a team and ManickaPJuly 3, 2024 18:23
@MihaZupanMihaZupan
self-assigned this
Jul 3, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Jul 3, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 3, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
rzikm
rzikm
approved these changes
Jul 4, 2024
View reviewed changes
@rzikmrzikm
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.

LGTM, thanks!
ManickaP
ManickaP
reviewed
Jul 4, 2024
View reviewed changes
@ManickaPManickaP
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.

Before this PR, WritesClosed was handled right after completing writes, logically next to each other. Now, the logic is split in different places. I know it's more efficient, but IMHO it's much harder to follow now and not mess up in the future.
I'll not block this PR on this, I'm just sharing my perspective.
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated


This was referenced Jul 5, 2024

Open


Closed


Closed

@MihaZupan
MihaZupan
commented
Jul 6, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 6, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 2d92034
into
dotnet:mainJul 8, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 9, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@ManickaPManickaPManickaP approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Quic

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MihaZupan@ManickaP@rzikm

, '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
Reduce allocations in Quic/HTTP3 - #104394
Merged
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Jul 8, 2024
Merged
Reduce allocations in Quic/HTTP3#104394
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Conversation
@MihaZupan
@MihaZupanMihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

Member
As of #104035 we're now waiting for WritesClosed on every request, and paying for the allocations backing that Task. This PR attempts to claw that perf back.
This does a couple of things:
  • if the ResettableValueTaskSource's final task was already signaled by the time someone first asked for the Task, avoid allocating the intermediate TaskCompletionSource and its Task.
  • Move the waiting on WritesClosed later on in Http3's SendAsync (after receiving the response) to make it more likely that WritesClosed will have been completed by the time we check, thus avoiding the allocations.
  • Honors the cancellationToken while waiting for WritesClosed -- @ManickaP I'm guessing that's desirable here?
  • Avoids the state machine allocation in FlushSendBufferAsync
@MihaZupanMihaZupan
added this to the 9.0.0 milestone
Jul 3, 2024
@MihaZupan
MihaZupan
requested review from
a team and ManickaPJuly 3, 2024 18:23
@MihaZupanMihaZupan
self-assigned this
Jul 3, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Jul 3, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 3, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
rzikm
rzikm
approved these changes
Jul 4, 2024
View reviewed changes
@rzikmrzikm
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.

LGTM, thanks!
ManickaP
ManickaP
reviewed
Jul 4, 2024
View reviewed changes
@ManickaPManickaP
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.

Before this PR, WritesClosed was handled right after completing writes, logically next to each other. Now, the logic is split in different places. I know it's more efficient, but IMHO it's much harder to follow now and not mess up in the future.
I'll not block this PR on this, I'm just sharing my perspective.
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated


This was referenced Jul 5, 2024

Open


Closed


Closed

@MihaZupan
MihaZupan
commented
Jul 6, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 6, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 2d92034
into
dotnet:mainJul 8, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 9, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@ManickaPManickaPManickaP approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Quic

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MihaZupan@ManickaP@rzikm

, '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
Reduce allocations in Quic/HTTP3 - #104394
Merged
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Jul 8, 2024
Merged
Reduce allocations in Quic/HTTP3#104394
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Conversation
@MihaZupan
@MihaZupanMihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

Member
As of #104035 we're now waiting for WritesClosed on every request, and paying for the allocations backing that Task. This PR attempts to claw that perf back.
This does a couple of things:
  • if the ResettableValueTaskSource's final task was already signaled by the time someone first asked for the Task, avoid allocating the intermediate TaskCompletionSource and its Task.
  • Move the waiting on WritesClosed later on in Http3's SendAsync (after receiving the response) to make it more likely that WritesClosed will have been completed by the time we check, thus avoiding the allocations.
  • Honors the cancellationToken while waiting for WritesClosed -- @ManickaP I'm guessing that's desirable here?
  • Avoids the state machine allocation in FlushSendBufferAsync
@MihaZupanMihaZupan
added this to the 9.0.0 milestone
Jul 3, 2024
@MihaZupan
MihaZupan
requested review from
a team and ManickaPJuly 3, 2024 18:23
@MihaZupanMihaZupan
self-assigned this
Jul 3, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Jul 3, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 3, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
rzikm
rzikm
approved these changes
Jul 4, 2024
View reviewed changes
@rzikmrzikm
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.

LGTM, thanks!
ManickaP
ManickaP
reviewed
Jul 4, 2024
View reviewed changes
@ManickaPManickaP
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.

Before this PR, WritesClosed was handled right after completing writes, logically next to each other. Now, the logic is split in different places. I know it's more efficient, but IMHO it's much harder to follow now and not mess up in the future.
I'll not block this PR on this, I'm just sharing my perspective.
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated


This was referenced Jul 5, 2024

Open


Closed


Closed

@MihaZupan
MihaZupan
commented
Jul 6, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 6, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 2d92034
into
dotnet:mainJul 8, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 9, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@ManickaPManickaPManickaP approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Quic

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MihaZupan@ManickaP@rzikm

, '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
Reduce allocations in Quic/HTTP3 - #104394
Merged
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Jul 8, 2024
Merged
Reduce allocations in Quic/HTTP3#104394
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Conversation
@MihaZupan
@MihaZupanMihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

Member
As of #104035 we're now waiting for WritesClosed on every request, and paying for the allocations backing that Task. This PR attempts to claw that perf back.
This does a couple of things:
  • if the ResettableValueTaskSource's final task was already signaled by the time someone first asked for the Task, avoid allocating the intermediate TaskCompletionSource and its Task.
  • Move the waiting on WritesClosed later on in Http3's SendAsync (after receiving the response) to make it more likely that WritesClosed will have been completed by the time we check, thus avoiding the allocations.
  • Honors the cancellationToken while waiting for WritesClosed -- @ManickaP I'm guessing that's desirable here?
  • Avoids the state machine allocation in FlushSendBufferAsync
@MihaZupanMihaZupan
added this to the 9.0.0 milestone
Jul 3, 2024
@MihaZupan
MihaZupan
requested review from
a team and ManickaPJuly 3, 2024 18:23
@MihaZupanMihaZupan
self-assigned this
Jul 3, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Jul 3, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 3, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
rzikm
rzikm
approved these changes
Jul 4, 2024
View reviewed changes
@rzikmrzikm
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.

LGTM, thanks!
ManickaP
ManickaP
reviewed
Jul 4, 2024
View reviewed changes
@ManickaPManickaP
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.

Before this PR, WritesClosed was handled right after completing writes, logically next to each other. Now, the logic is split in different places. I know it's more efficient, but IMHO it's much harder to follow now and not mess up in the future.
I'll not block this PR on this, I'm just sharing my perspective.
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated


This was referenced Jul 5, 2024

Open


Closed


Closed

@MihaZupan
MihaZupan
commented
Jul 6, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 6, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 2d92034
into
dotnet:mainJul 8, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 9, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@ManickaPManickaPManickaP approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Quic

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MihaZupan@ManickaP@rzikm

, '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
Reduce allocations in Quic/HTTP3 - #104394
Merged
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Jul 8, 2024
Merged
Reduce allocations in Quic/HTTP3#104394
MihaZupan merged 2 commits into
dotnet:mainfrom 
MihaZupan:quic-allocs
Conversation
@MihaZupan
@MihaZupanMihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

Member
As of #104035 we're now waiting for WritesClosed on every request, and paying for the allocations backing that Task. This PR attempts to claw that perf back.
This does a couple of things:
  • if the ResettableValueTaskSource's final task was already signaled by the time someone first asked for the Task, avoid allocating the intermediate TaskCompletionSource and its Task.
  • Move the waiting on WritesClosed later on in Http3's SendAsync (after receiving the response) to make it more likely that WritesClosed will have been completed by the time we check, thus avoiding the allocations.
  • Honors the cancellationToken while waiting for WritesClosed -- @ManickaP I'm guessing that's desirable here?
  • Avoids the state machine allocation in FlushSendBufferAsync
@MihaZupanMihaZupan
added this to the 9.0.0 milestone
Jul 3, 2024
@MihaZupan
MihaZupan
requested review from
a team and ManickaPJuly 3, 2024 18:23
@MihaZupanMihaZupan
self-assigned this
Jul 3, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Jul 3, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
@MihaZupan
MihaZupan
commented
Jul 3, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 3, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
rzikm
rzikm
approved these changes
Jul 4, 2024
View reviewed changes
@rzikmrzikm
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.

LGTM, thanks!
ManickaP
ManickaP
reviewed
Jul 4, 2024
View reviewed changes
@ManickaPManickaP
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.

Before this PR, WritesClosed was handled right after completing writes, logically next to each other. Now, the logic is split in different places. I know it's more efficient, but IMHO it's much harder to follow now and not mess up in the future.
I'll not block this PR on this, I'm just sharing my perspective.
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated


This was referenced Jul 5, 2024

Open


Closed


Closed

@MihaZupan
MihaZupan
commented
Jul 6, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jul 6, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 2d92034
into
dotnet:mainJul 8, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 9, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@ManickaPManickaPManickaP approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

@MihaZupanMihaZupan

Labels

area-System.Net.Quic

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MihaZupan@ManickaP@rzikm