Refactor MsQuic's native IP address types. - #53461

Merged
wfurt merged 3 commits into
dotnet:mainfrom
teo-tsirpanis:patch-1
May 31, 2021
Merged

Refactor MsQuic's native IP address types.#53461
wfurt merged 3 commits into
dotnet:mainfrom
teo-tsirpanis:patch-1

Conversation

@teo-tsirpanis

@teo-tsirpanisteo-tsirpanis commented May 29, 2021

Copy link
Copy Markdown
Contributor

MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.

This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.

A similar trick is already performed on the spans returned by the System.GCMemory type.

@ghostghost added the area-System.Net label May 29, 2021
@ghost

ghost commented May 29, 2021

Copy link
Copy Markdown

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

Issue Details

MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.

This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.

A similar trick is already performed on the spans returned by the System.GCMemory type.

Author:teo-tsirpanis
Assignees:-
Labels:

area-System.Net

Milestone:-

stephentoub
stephentoub approved these changes May 29, 2021
View reviewed changes

@stephentoubstephentoub 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, based on current usage. This becomes very unsafe if that span is passed out to a different scope than the struct was defined, so usage of this property needs to be scrutinized heavily moving forward.

@teo-tsirpanis

teo-tsirpanis commented May 29, 2021

Copy link
Copy Markdown
ContributorAuthor

I got another idea. We could add a ToIpAddress method at each struct. The span would not be exposed and INetToIPAddresswould call the method on the appropriate struct depending on whether IPv4 or 6 is used.

@stephentoub

stephentoub commented May 29, 2021

Copy link
Copy Markdown
Member

My preference actually would be for the _addr fields to instead by a fixed buffer pointer (e.g. internal fixed byte sin_addr[16];), and then the call site would construct the span from that pointer. That keeps layering cleaner, reduces the cruft in the struct definition, and more closely matches the native header file.

@teo-tsirpanisteo-tsirpanis changed the title Avoid byte array allocations on MsQuic's IP address structs.Refactor MsQuic's native IP address types.May 29, 2021
@teo-tsirpanis

teo-tsirpanis commented May 29, 2021

Copy link
Copy Markdown
ContributorAuthor

OK @stephentoub, I applied your suggestions; they turned out to allow optimizing the IPEndPointToINet method as well. Could you review it again?

stephentoub
stephentoub reviewed May 30, 2021
View reviewed changes
Comment thread...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs Outdated
Comment thread.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs Outdated
@teo-tsirpanis
Use fixed buffers in MsQuic's native IP address types.
07bc4e2
And handle them more efficiently when moving between them and .NET's IPAddresses.
@teo-tsirpanis
Update the names of all SOCKADDR_IN6's fields.
ba747cb
@stephentoub

stephentoub commented May 30, 2021

Copy link
Copy Markdown
Member

cc: @dotnet/ncl

wfurt
wfurt approved these changes May 31, 2021
View reviewed changes

@wfurtwfurt 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

@wfurt
wfurt merged commit 3846a9c into dotnet:mainMay 31, 2021
@teo-tsirpanis
teo-tsirpanis deleted the patch-1 branch May 31, 2021 10:01
@wfurtwfurt mentioned this pull request May 31, 2021
Closed
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@AustinWiseAustinWise mentioned this pull request Jun 12, 2021
Merged
ManickaP pushed a commit that referenced this pull request Jun 14, 2021
@AustinWise
Remove allocation from MsQuicAddressHelpers.IPEndPointToINet (#54095)
af5c238
Assuming that GetAddressBytes() does not have side effects, this call
should not be needed.
Clean up from #53461
@ghostghost locked as resolved and limited conversation to collaborators Jun 30, 2021
@karelzkarelz added this to the 6.0.0 milestone Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes
+2 more reviewers
@geoffkizergeoffkizergeoffkizer left review comments
@phocodesphocodesphocodes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@stephentoub@geoffkizer@wfurt@phocodes@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Refactor MsQuic's native IP address types. - #53461
Merged
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
May 31, 2021
Merged
Refactor MsQuic's native IP address types.#53461
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
Conversation
@teo-tsirpanis
@teo-tsirpanisteo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

Contributor
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
@ghostghost
added
the 
area-System.Net

label
May 29, 2021
@ghost
ghost
commented
May 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Issue Details
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
Author:teo-tsirpanis
Assignees:-
Labels:
area-System.Net
Milestone:-
stephentoub
stephentoub
approved these changes
May 29, 2021
View reviewed changes
@stephentoubstephentoub
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, based on current usage. This becomes very unsafe if that span is passed out to a different scope than the struct was defined, so usage of this property needs to be scrutinized heavily moving forward.
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
I got another idea. We could add a ToIpAddress method at each struct. The span would not be exposed and INetToIPAddresswould call the method on the appropriate struct depending on whether IPv4 or 6 is used.
@stephentoub
stephentoub
commented
May 29, 2021

Copy link


Copy Markdown

Member
My preference actually would be for the _addr fields to instead by a fixed buffer pointer (e.g. internal fixed byte sin_addr[16];), and then the call site would construct the span from that pointer. That keeps layering cleaner, reduces the cruft in the struct definition, and more closely matches the native header file.
@teo-tsirpanisteo-tsirpanis
changed the title
Avoid byte array allocations on MsQuic's IP address structs.Refactor MsQuic's native IP address types.May 29, 2021
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
OK @stephentoub, I applied your suggestions; they turned out to allow optimizing the IPEndPointToINet method as well. Could you review it again?
stephentoub
stephentoub
reviewed
May 30, 2021
View reviewed changes
Comment thread...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs
Outdated

Comment thread.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs
Outdated

@teo-tsirpanis
Use fixed buffers in MsQuic's native IP address types.
07bc4e2
And handle them more efficiently when moving between them and .NET's IPAddresses.
@teo-tsirpanis
Update the names of all SOCKADDR_IN6's fields.
ba747cb
@stephentoub
stephentoub
commented
May 30, 2021

Copy link


Copy Markdown

Member
cc: @dotnet/ncl
wfurt
wfurt
approved these changes
May 31, 2021
View reviewed changes
@wfurtwfurt
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
@wfurt
wfurt
merged commit 3846a9c
into
dotnet:mainMay 31, 2021
@teo-tsirpanis
teo-tsirpanis
deleted the

patch-1

branch
May 31, 2021 10:01
@wfurtwfurt
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@AustinWiseAustinWise
mentioned this pull request
Jun 12, 2021
 Merged

ManickaP pushed a commit
that referenced
this pull request
Jun 14, 2021
@AustinWise
Remove allocation from MsQuicAddressHelpers.IPEndPointToINet (#54095)
af5c238
Assuming that GetAddressBytes() does not have side effects, this call
should not be needed.
Clean up from #53461
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@phocodesphocodesphocodes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@stephentoub@geoffkizer@wfurt@phocodes@karelz

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

Skip to content
Refactor MsQuic's native IP address types. - #53461
Merged
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
May 31, 2021
Merged
Refactor MsQuic's native IP address types.#53461
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
Conversation
@teo-tsirpanis
@teo-tsirpanisteo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

Contributor
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
@ghostghost
added
the 
area-System.Net

label
May 29, 2021
@ghost
ghost
commented
May 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Issue Details
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
Author:teo-tsirpanis
Assignees:-
Labels:
area-System.Net
Milestone:-
stephentoub
stephentoub
approved these changes
May 29, 2021
View reviewed changes
@stephentoubstephentoub
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, based on current usage. This becomes very unsafe if that span is passed out to a different scope than the struct was defined, so usage of this property needs to be scrutinized heavily moving forward.
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
I got another idea. We could add a ToIpAddress method at each struct. The span would not be exposed and INetToIPAddresswould call the method on the appropriate struct depending on whether IPv4 or 6 is used.
@stephentoub
stephentoub
commented
May 29, 2021

Copy link


Copy Markdown

Member
My preference actually would be for the _addr fields to instead by a fixed buffer pointer (e.g. internal fixed byte sin_addr[16];), and then the call site would construct the span from that pointer. That keeps layering cleaner, reduces the cruft in the struct definition, and more closely matches the native header file.
@teo-tsirpanisteo-tsirpanis
changed the title
Avoid byte array allocations on MsQuic's IP address structs.Refactor MsQuic's native IP address types.May 29, 2021
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
OK @stephentoub, I applied your suggestions; they turned out to allow optimizing the IPEndPointToINet method as well. Could you review it again?
stephentoub
stephentoub
reviewed
May 30, 2021
View reviewed changes
Comment thread...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs
Outdated

Comment thread.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs
Outdated

@teo-tsirpanis
Use fixed buffers in MsQuic's native IP address types.
07bc4e2
And handle them more efficiently when moving between them and .NET's IPAddresses.
@teo-tsirpanis
Update the names of all SOCKADDR_IN6's fields.
ba747cb
@stephentoub
stephentoub
commented
May 30, 2021

Copy link


Copy Markdown

Member
cc: @dotnet/ncl
wfurt
wfurt
approved these changes
May 31, 2021
View reviewed changes
@wfurtwfurt
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
@wfurt
wfurt
merged commit 3846a9c
into
dotnet:mainMay 31, 2021
@teo-tsirpanis
teo-tsirpanis
deleted the

patch-1

branch
May 31, 2021 10:01
@wfurtwfurt
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@AustinWiseAustinWise
mentioned this pull request
Jun 12, 2021
 Merged

ManickaP pushed a commit
that referenced
this pull request
Jun 14, 2021
@AustinWise
Remove allocation from MsQuicAddressHelpers.IPEndPointToINet (#54095)
af5c238
Assuming that GetAddressBytes() does not have side effects, this call
should not be needed.
Clean up from #53461
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@phocodesphocodesphocodes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@stephentoub@geoffkizer@wfurt@phocodes@karelz

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

Skip to content
Refactor MsQuic's native IP address types. - #53461
Merged
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
May 31, 2021
Merged
Refactor MsQuic's native IP address types.#53461
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
Conversation
@teo-tsirpanis
@teo-tsirpanisteo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

Contributor
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
@ghostghost
added
the 
area-System.Net

label
May 29, 2021
@ghost
ghost
commented
May 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Issue Details
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
Author:teo-tsirpanis
Assignees:-
Labels:
area-System.Net
Milestone:-
stephentoub
stephentoub
approved these changes
May 29, 2021
View reviewed changes
@stephentoubstephentoub
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, based on current usage. This becomes very unsafe if that span is passed out to a different scope than the struct was defined, so usage of this property needs to be scrutinized heavily moving forward.
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
I got another idea. We could add a ToIpAddress method at each struct. The span would not be exposed and INetToIPAddresswould call the method on the appropriate struct depending on whether IPv4 or 6 is used.
@stephentoub
stephentoub
commented
May 29, 2021

Copy link


Copy Markdown

Member
My preference actually would be for the _addr fields to instead by a fixed buffer pointer (e.g. internal fixed byte sin_addr[16];), and then the call site would construct the span from that pointer. That keeps layering cleaner, reduces the cruft in the struct definition, and more closely matches the native header file.
@teo-tsirpanisteo-tsirpanis
changed the title
Avoid byte array allocations on MsQuic's IP address structs.Refactor MsQuic's native IP address types.May 29, 2021
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
OK @stephentoub, I applied your suggestions; they turned out to allow optimizing the IPEndPointToINet method as well. Could you review it again?
stephentoub
stephentoub
reviewed
May 30, 2021
View reviewed changes
Comment thread...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs
Outdated

Comment thread.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs
Outdated

@teo-tsirpanis
Use fixed buffers in MsQuic's native IP address types.
07bc4e2
And handle them more efficiently when moving between them and .NET's IPAddresses.
@teo-tsirpanis
Update the names of all SOCKADDR_IN6's fields.
ba747cb
@stephentoub
stephentoub
commented
May 30, 2021

Copy link


Copy Markdown

Member
cc: @dotnet/ncl
wfurt
wfurt
approved these changes
May 31, 2021
View reviewed changes
@wfurtwfurt
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
@wfurt
wfurt
merged commit 3846a9c
into
dotnet:mainMay 31, 2021
@teo-tsirpanis
teo-tsirpanis
deleted the

patch-1

branch
May 31, 2021 10:01
@wfurtwfurt
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@AustinWiseAustinWise
mentioned this pull request
Jun 12, 2021
 Merged

ManickaP pushed a commit
that referenced
this pull request
Jun 14, 2021
@AustinWise
Remove allocation from MsQuicAddressHelpers.IPEndPointToINet (#54095)
af5c238
Assuming that GetAddressBytes() does not have side effects, this call
should not be needed.
Clean up from #53461
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@phocodesphocodesphocodes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@stephentoub@geoffkizer@wfurt@phocodes@karelz

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

Skip to content
Refactor MsQuic's native IP address types. - #53461
Merged
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
May 31, 2021
Merged
Refactor MsQuic's native IP address types.#53461
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
Conversation
@teo-tsirpanis
@teo-tsirpanisteo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

Contributor
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
@ghostghost
added
the 
area-System.Net

label
May 29, 2021
@ghost
ghost
commented
May 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Issue Details
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
Author:teo-tsirpanis
Assignees:-
Labels:
area-System.Net
Milestone:-
stephentoub
stephentoub
approved these changes
May 29, 2021
View reviewed changes
@stephentoubstephentoub
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, based on current usage. This becomes very unsafe if that span is passed out to a different scope than the struct was defined, so usage of this property needs to be scrutinized heavily moving forward.
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
I got another idea. We could add a ToIpAddress method at each struct. The span would not be exposed and INetToIPAddresswould call the method on the appropriate struct depending on whether IPv4 or 6 is used.
@stephentoub
stephentoub
commented
May 29, 2021

Copy link


Copy Markdown

Member
My preference actually would be for the _addr fields to instead by a fixed buffer pointer (e.g. internal fixed byte sin_addr[16];), and then the call site would construct the span from that pointer. That keeps layering cleaner, reduces the cruft in the struct definition, and more closely matches the native header file.
@teo-tsirpanisteo-tsirpanis
changed the title
Avoid byte array allocations on MsQuic's IP address structs.Refactor MsQuic's native IP address types.May 29, 2021
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
OK @stephentoub, I applied your suggestions; they turned out to allow optimizing the IPEndPointToINet method as well. Could you review it again?
stephentoub
stephentoub
reviewed
May 30, 2021
View reviewed changes
Comment thread...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs
Outdated

Comment thread.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs
Outdated

@teo-tsirpanis
Use fixed buffers in MsQuic's native IP address types.
07bc4e2
And handle them more efficiently when moving between them and .NET's IPAddresses.
@teo-tsirpanis
Update the names of all SOCKADDR_IN6's fields.
ba747cb
@stephentoub
stephentoub
commented
May 30, 2021

Copy link


Copy Markdown

Member
cc: @dotnet/ncl
wfurt
wfurt
approved these changes
May 31, 2021
View reviewed changes
@wfurtwfurt
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
@wfurt
wfurt
merged commit 3846a9c
into
dotnet:mainMay 31, 2021
@teo-tsirpanis
teo-tsirpanis
deleted the

patch-1

branch
May 31, 2021 10:01
@wfurtwfurt
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@AustinWiseAustinWise
mentioned this pull request
Jun 12, 2021
 Merged

ManickaP pushed a commit
that referenced
this pull request
Jun 14, 2021
@AustinWise
Remove allocation from MsQuicAddressHelpers.IPEndPointToINet (#54095)
af5c238
Assuming that GetAddressBytes() does not have side effects, this call
should not be needed.
Clean up from #53461
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@phocodesphocodesphocodes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@stephentoub@geoffkizer@wfurt@phocodes@karelz

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

Skip to content
Refactor MsQuic's native IP address types. - #53461
Merged
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
May 31, 2021
Merged
Refactor MsQuic's native IP address types.#53461
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
Conversation
@teo-tsirpanis
@teo-tsirpanisteo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

Contributor
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
@ghostghost
added
the 
area-System.Net

label
May 29, 2021
@ghost
ghost
commented
May 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Issue Details
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
Author:teo-tsirpanis
Assignees:-
Labels:
area-System.Net
Milestone:-
stephentoub
stephentoub
approved these changes
May 29, 2021
View reviewed changes
@stephentoubstephentoub
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, based on current usage. This becomes very unsafe if that span is passed out to a different scope than the struct was defined, so usage of this property needs to be scrutinized heavily moving forward.
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
I got another idea. We could add a ToIpAddress method at each struct. The span would not be exposed and INetToIPAddresswould call the method on the appropriate struct depending on whether IPv4 or 6 is used.
@stephentoub
stephentoub
commented
May 29, 2021

Copy link


Copy Markdown

Member
My preference actually would be for the _addr fields to instead by a fixed buffer pointer (e.g. internal fixed byte sin_addr[16];), and then the call site would construct the span from that pointer. That keeps layering cleaner, reduces the cruft in the struct definition, and more closely matches the native header file.
@teo-tsirpanisteo-tsirpanis
changed the title
Avoid byte array allocations on MsQuic's IP address structs.Refactor MsQuic's native IP address types.May 29, 2021
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
OK @stephentoub, I applied your suggestions; they turned out to allow optimizing the IPEndPointToINet method as well. Could you review it again?
stephentoub
stephentoub
reviewed
May 30, 2021
View reviewed changes
Comment thread...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs
Outdated

Comment thread.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs
Outdated

@teo-tsirpanis
Use fixed buffers in MsQuic's native IP address types.
07bc4e2
And handle them more efficiently when moving between them and .NET's IPAddresses.
@teo-tsirpanis
Update the names of all SOCKADDR_IN6's fields.
ba747cb
@stephentoub
stephentoub
commented
May 30, 2021

Copy link


Copy Markdown

Member
cc: @dotnet/ncl
wfurt
wfurt
approved these changes
May 31, 2021
View reviewed changes
@wfurtwfurt
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
@wfurt
wfurt
merged commit 3846a9c
into
dotnet:mainMay 31, 2021
@teo-tsirpanis
teo-tsirpanis
deleted the

patch-1

branch
May 31, 2021 10:01
@wfurtwfurt
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@AustinWiseAustinWise
mentioned this pull request
Jun 12, 2021
 Merged

ManickaP pushed a commit
that referenced
this pull request
Jun 14, 2021
@AustinWise
Remove allocation from MsQuicAddressHelpers.IPEndPointToINet (#54095)
af5c238
Assuming that GetAddressBytes() does not have side effects, this call
should not be needed.
Clean up from #53461
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@phocodesphocodesphocodes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@stephentoub@geoffkizer@wfurt@phocodes@karelz

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

Skip to content
Refactor MsQuic's native IP address types. - #53461
Merged
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
May 31, 2021
Merged
Refactor MsQuic's native IP address types.#53461
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
Conversation
@teo-tsirpanis
@teo-tsirpanisteo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

Contributor
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
@ghostghost
added
the 
area-System.Net

label
May 29, 2021
@ghost
ghost
commented
May 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Issue Details
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
Author:teo-tsirpanis
Assignees:-
Labels:
area-System.Net
Milestone:-
stephentoub
stephentoub
approved these changes
May 29, 2021
View reviewed changes
@stephentoubstephentoub
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, based on current usage. This becomes very unsafe if that span is passed out to a different scope than the struct was defined, so usage of this property needs to be scrutinized heavily moving forward.
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
I got another idea. We could add a ToIpAddress method at each struct. The span would not be exposed and INetToIPAddresswould call the method on the appropriate struct depending on whether IPv4 or 6 is used.
@stephentoub
stephentoub
commented
May 29, 2021

Copy link


Copy Markdown

Member
My preference actually would be for the _addr fields to instead by a fixed buffer pointer (e.g. internal fixed byte sin_addr[16];), and then the call site would construct the span from that pointer. That keeps layering cleaner, reduces the cruft in the struct definition, and more closely matches the native header file.
@teo-tsirpanisteo-tsirpanis
changed the title
Avoid byte array allocations on MsQuic's IP address structs.Refactor MsQuic's native IP address types.May 29, 2021
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
OK @stephentoub, I applied your suggestions; they turned out to allow optimizing the IPEndPointToINet method as well. Could you review it again?
stephentoub
stephentoub
reviewed
May 30, 2021
View reviewed changes
Comment thread...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs
Outdated

Comment thread.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs
Outdated

@teo-tsirpanis
Use fixed buffers in MsQuic's native IP address types.
07bc4e2
And handle them more efficiently when moving between them and .NET's IPAddresses.
@teo-tsirpanis
Update the names of all SOCKADDR_IN6's fields.
ba747cb
@stephentoub
stephentoub
commented
May 30, 2021

Copy link


Copy Markdown

Member
cc: @dotnet/ncl
wfurt
wfurt
approved these changes
May 31, 2021
View reviewed changes
@wfurtwfurt
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
@wfurt
wfurt
merged commit 3846a9c
into
dotnet:mainMay 31, 2021
@teo-tsirpanis
teo-tsirpanis
deleted the

patch-1

branch
May 31, 2021 10:01
@wfurtwfurt
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@AustinWiseAustinWise
mentioned this pull request
Jun 12, 2021
 Merged

ManickaP pushed a commit
that referenced
this pull request
Jun 14, 2021
@AustinWise
Remove allocation from MsQuicAddressHelpers.IPEndPointToINet (#54095)
af5c238
Assuming that GetAddressBytes() does not have side effects, this call
should not be needed.
Clean up from #53461
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@phocodesphocodesphocodes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@stephentoub@geoffkizer@wfurt@phocodes@karelz

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

Skip to content
Refactor MsQuic's native IP address types. - #53461
Merged
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
May 31, 2021
Merged
Refactor MsQuic's native IP address types.#53461
wfurt merged 3 commits into
dotnet:mainfrom 
teo-tsirpanis:patch-1
Conversation
@teo-tsirpanis
@teo-tsirpanisteo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

Contributor
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
@ghostghost
added
the 
area-System.Net

label
May 29, 2021
@ghost
ghost
commented
May 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
Issue Details
MsQuicAddressHelpers.INetToIPEndPoint passes byte arrays to the IPAddress' constructor which were allocated by the Address properties of the MsQuicNativeMethods+SOCKADDR_IN(6) structs. There are no other uses of these properties.
This PR changes these properties to return read-only spans, avoiding the allocation of an byte array.
A similar trick is already performed on the spans returned by the System.GCMemory type.
Author:teo-tsirpanis
Assignees:-
Labels:
area-System.Net
Milestone:-
stephentoub
stephentoub
approved these changes
May 29, 2021
View reviewed changes
@stephentoubstephentoub
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, based on current usage. This becomes very unsafe if that span is passed out to a different scope than the struct was defined, so usage of this property needs to be scrutinized heavily moving forward.
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
I got another idea. We could add a ToIpAddress method at each struct. The span would not be exposed and INetToIPAddresswould call the method on the appropriate struct depending on whether IPv4 or 6 is used.
@stephentoub
stephentoub
commented
May 29, 2021

Copy link


Copy Markdown

Member
My preference actually would be for the _addr fields to instead by a fixed buffer pointer (e.g. internal fixed byte sin_addr[16];), and then the call site would construct the span from that pointer. That keeps layering cleaner, reduces the cruft in the struct definition, and more closely matches the native header file.
@teo-tsirpanisteo-tsirpanis
changed the title
Avoid byte array allocations on MsQuic's IP address structs.Refactor MsQuic's native IP address types.May 29, 2021
@teo-tsirpanis
teo-tsirpanis
commented
May 29, 2021

Copy link


Copy Markdown

ContributorAuthor
OK @stephentoub, I applied your suggestions; they turned out to allow optimizing the IPEndPointToINet method as well. Could you review it again?
stephentoub
stephentoub
reviewed
May 30, 2021
View reviewed changes
Comment thread...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs
Outdated

Comment thread.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs
Outdated

@teo-tsirpanis
Use fixed buffers in MsQuic's native IP address types.
07bc4e2
And handle them more efficiently when moving between them and .NET's IPAddresses.
@teo-tsirpanis
Update the names of all SOCKADDR_IN6's fields.
ba747cb
@stephentoub
stephentoub
commented
May 30, 2021

Copy link


Copy Markdown

Member
cc: @dotnet/ncl
wfurt
wfurt
approved these changes
May 31, 2021
View reviewed changes
@wfurtwfurt
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
@wfurt
wfurt
merged commit 3846a9c
into
dotnet:mainMay 31, 2021
@teo-tsirpanis
teo-tsirpanis
deleted the

patch-1

branch
May 31, 2021 10:01
@wfurtwfurt
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@AustinWiseAustinWise
mentioned this pull request
Jun 12, 2021
 Merged

ManickaP pushed a commit
that referenced
this pull request
Jun 14, 2021
@AustinWise
Remove allocation from MsQuicAddressHelpers.IPEndPointToINet (#54095)
af5c238
Assuming that GetAddressBytes() does not have side effects, this call
should not be needed.
Clean up from #53461
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@phocodesphocodesphocodes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@teo-tsirpanis@stephentoub@geoffkizer@wfurt@phocodes@karelz