Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit - #102547

Closed
jww3 wants to merge 10 commits into
dotnet:mainfrom
jww3:patch-1
Closed

Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit#102547
jww3 wants to merge 10 commits into
dotnet:mainfrom
jww3:patch-1

Conversation

@jww3

@jww3jww3 commented May 22, 2024•
edited
Loading

Copy link
Copy Markdown

Added the following comment to clarify the rationale for using | over || in Char::IsAsciiLetterOrDigit.

// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 22, 2024
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label May 22, 2024
@huoyaoyuan

huoyaoyuan commented May 22, 2024•
edited
Loading

Copy link
Copy Markdown
Member

This can lead to the right-hand side of the expression being evaluated unnecessarily.

The JIT is able to optimize this case because the functions in two branches are trivial.

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHjykDYBAgAbBgElcAQVxgAlvIAyMDBhhQAFGAAW2KAzABKBgF4AfA00BXeQDsMRzdoaEGABgSl3JuAwDk2P4mADymAQBe/gx+gf4A3Fw89EwCQiLiUgBCqgDuMDB22noGYGiGJQz49hJ2YKLWuPIAbjDluvpV2Ai19Y0tMCYWSdw29o4uftV2vQ1NraHhYw5O+N2z/a0xVTV1cwNGidQ8DCO8qYLCYpIycooqahoA8lAAIvIA5vIYxZ3GZpYpLIFMpVOotP83Nk8gUimUAu5/OV/ABOYJHE5nFL8S4ZG7A+5g55vT7fUi/UpDQG3EEPcHaEyEKG4HIYfKFbTIxHItGHGgAXyAA===

| and || provides the same assembly code, but || generates slightly longer IL.

@PaulusParssinen

PaulusParssinen commented May 22, 2024

Copy link
Copy Markdown
Contributor

This is intentional, see https://github.com/dotnet/runtime/pull/69318/files#r872595998

@jww3

jww3 commented May 22, 2024

Copy link
Copy Markdown
Author

Ah, I see. Thanks for clarifying! Maybe we should add a comment so that we don't keep revisiting this?

@PaulusParssinen

PaulusParssinen commented May 22, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

In my opinion it does warrant a comment so people don't need to pull out sharplab/disasmo to check why it's like this 😆

I think the rationale here is that the input is not considered to be predictable so we don't try have our stupidly smart modern branch predictors predict it(?). If one case is more common the checks could be ordered and the other version could be tiny bit faster (I think this sorta cmov data-dependecy issue vs a branch in a loop.. but kinda not as the current version doesn't even need conditional mov so it's even better like this..)

Also the IL is much nicer for the importer in the branchless version.

@huoyaoyuan

huoyaoyuan commented May 22, 2024

Copy link
Copy Markdown
Member

Those operators evaluate the right-hand operand only if it's necessary.

In this case, branching itself has more overhead than evaluating either branch.
For short-circuiting, we usually care more about the side effect of evaluating a branch, including triggering NullReferenceException.

This can be a common sense of advanced optimization, but sometimes I can't notice it.

@jww3

jww3 commented May 22, 2024

Copy link
Copy Markdown
Author

Thanks! I'll convert this PR into just a PR to add an explanatory comment.

@build-analysisbuild-analysisBot mentioned this pull request May 22, 2024
Closed
@jww3jww3 changed the title Use short-circuiting || operator in Char::IsAsciiLetterOrDigitDocument the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
@jww3jww3 changed the title Document the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitDocument the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
huoyaoyuan
huoyaoyuan reviewed May 22, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs Outdated
Comment on lines +277 to +279
// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@huoyaoyuanhuoyaoyuanMay 22, 2024

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.

The comment looks too verbose for users who can understand it. The position is also questionable with xml doc.

I'd suggest a trailing comment on the same line, just saying "use arithmetic or to avoid branching" or similar.

This was referenced May 22, 2024
Closed
Closed
@build-analysisbuild-analysisBot mentioned this pull request May 23, 2024
Closed
@jww3

jww3 commented May 23, 2024

Copy link
Copy Markdown
Author

Tests seem to be failing for an unrelated reason. (In the end, all I did was add a comment.) I don't seem to have permission to schedule a retry of the tests.

Any advice would be appreciated.

stephentoub
stephentoub reviewed Jun 23, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
/// 'a' through 'z', inclusive, or '0' through '9', inclusive.
/// </remarks>
public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9');
public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9'); // Using | here is an optimization (avoids branching).

@stephentoubstephentoubJun 23, 2024

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.

Thanks. There are a variety of places we use | like this, which is part of its raison d'etre. Why without a comment is there an assumption that it's a mistake and why is this particular use special so as to warrant a dedicated comment?

@jww3jww3Jul 31, 2024•
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

When I do a quick search through this repo for " | ", I don't see many instances of | being used for boolean operations.

If there were hundreds (or even dozens) of similar usage patterns, I'd agree that it's a common-enough practice as to not warrant any clarification.

It seems, however, there are only a few.

The net effect is that the codebase exhibits these occasional inconsistencies. Mentally, when you encounter one, it gives you pause: "Wait. That doesn't look right. Why's that?"

Maybe you remember why it's that way. Maybe you have to jog your memory. Maybe you're stumped. (Most people would be stumped.)

Either way, you've wasted time contemplating it and you're likely to do it all over again when you re-encounter it at some in the future.

A simple comment here short-circuits (pun intended) that whole mental exercise.

If there are indeed only a few such instances within this repo, I'd be happy to amend this PR to cover those as well.

@stephentoubstephentoubAug 18, 2024

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.

Thanks. Personally, I still don't believe this is valuable. The only reason to use a | here is for the exact reason the comment would state.

@jww3jww3Aug 23, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My guess is that most C# programmers aren't even aware that this is valid syntax for a boolean expression.

If you put yourself in their 👞👞 ...?

@am11am11 added area-System.Runtime needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 19, 2024
@dotnet-policy-servicedotnet-policy-serviceBot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 31, 2024
@build-analysisbuild-analysisBot mentioned this pull request Jul 31, 2024
Closed
3 tasks
This was referenced Jul 31, 2024
Open
Closed
Closed
@build-analysisbuild-analysisBot mentioned this pull request Aug 18, 2024
Closed
@jww3jww3 closed this Aug 23, 2024
This was referenced Aug 23, 2024
Open
Closed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Sep 23, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
+1 more reviewer
@huoyaoyuanhuoyaoyuanhuoyaoyuan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jww3@huoyaoyuan@PaulusParssinen@stephentoub@am11
, '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
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit - #102547
Closed
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Closed
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit#102547
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Conversation
@jww3
@jww3jww3
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Added the following comment to clarify the rationale for using | over || in Char::IsAsciiLetterOrDigit.
// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
May 22, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 22, 2024
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This can lead to the right-hand side of the expression being evaluated unnecessarily.
The JIT is able to optimize this case because the functions in two branches are trivial.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHjykDYBAgAbBgElcAQVxgAlvIAyMDBhhQAFGAAW2KAzABKBgF4AfA00BXeQDsMRzdoaEGABgSl3JuAwDk2P4mADymAQBe/gx+gf4A3Fw89EwCQiLiUgBCqgDuMDB22noGYGiGJQz49hJ2YKLWuPIAbjDluvpV2Ai19Y0tMCYWSdw29o4uftV2vQ1NraHhYw5O+N2z/a0xVTV1cwNGidQ8DCO8qYLCYpIycooqahoA8lAAIvIA5vIYxZ3GZpYpLIFMpVOotP83Nk8gUimUAu5/OV/ABOYJHE5nFL8S4ZG7A+5g55vT7fUi/UpDQG3EEPcHaEyEKG4HIYfKFbTIxHItGHGgAXyAA===
| and || provides the same assembly code, but || generates slightly longer IL.
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
This is intentional, see https://github.com/dotnet/runtime/pull/69318/files#r872595998
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Ah, I see. Thanks for clarifying! Maybe we should add a comment so that we don't keep revisiting this?
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
In my opinion it does warrant a comment so people don't need to pull out sharplab/disasmo to check why it's like this 😆
I think the rationale here is that the input is not considered to be predictable so we don't try have our stupidly smart modern branch predictors predict it(?). If one case is more common the checks could be ordered and the other version could be tiny bit faster (I think this sorta cmov data-dependecy issue vs a branch in a loop.. but kinda not as the current version doesn't even need conditional mov so it's even better like this..)
Also the IL is much nicer for the importer in the branchless version.
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024

Copy link


Copy Markdown

Member
Those operators evaluate the right-hand operand only if it's necessary.
In this case, branching itself has more overhead than evaluating either branch.

For short-circuiting, we usually care more about the side effect of evaluating a branch, including triggering NullReferenceException.
This can be a common sense of advanced optimization, but sometimes I can't notice it.
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Thanks! I'll convert this PR into just a PR to add an explanatory comment.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@jww3jww3
changed the title
Use short-circuiting || operator in Char::IsAsciiLetterOrDigitDocument the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
@jww3jww3
changed the title
Document the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitDocument the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
huoyaoyuan
huoyaoyuan
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
Outdated


Comment on lines

+277
 to 
+279

 // Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
 // It avoids a low-level branch instruction. (In other words, the overhead of branching
 // is greater than the cost of simply executing IsBetween.)
@huoyaoyuanhuoyaoyuanMay 22, 2024

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.

The comment looks too verbose for users who can understand it. The position is also questionable with xml doc.
I'd suggest a trailing comment on the same line, just saying "use arithmetic or to avoid branching" or similar.

This was referenced May 22, 2024

Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
May 23, 2024

Closed

@jww3
jww3
commented
May 23, 2024

Copy link


Copy Markdown

Author
Tests seem to be failing for an unrelated reason. (In the end, all I did was add a comment.) I don't seem to have permission to schedule a retry of the tests.
Any advice would be appreciated.
stephentoub
stephentoub
reviewed
Jun 23, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
 /// 'a' through 'z', inclusive, or '0' through '9', inclusive.
 /// </remarks>
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9');
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9'); // Using | here is an optimization (avoids branching).
@stephentoubstephentoubJun 23, 2024

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.

Thanks. There are a variety of places we use | like this, which is part of its raison d'etre. Why without a comment is there an assumption that it's a mistake and why is this particular use special so as to warrant a dedicated comment?
@jww3jww3Jul 31, 2024•

edited

Loading

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

When I do a quick search through this repo for " | ", I don't see many instances of | being used for boolean operations.
If there were hundreds (or even dozens) of similar usage patterns, I'd agree that it's a common-enough practice as to not warrant any clarification.
It seems, however, there are only a few.
The net effect is that the codebase exhibits these occasional inconsistencies. Mentally, when you encounter one, it gives you pause: "Wait. That doesn't look right. Why's that?"
Maybe you remember why it's that way. Maybe you have to jog your memory. Maybe you're stumped. (Most people would be stumped.)
Either way, you've wasted time contemplating it and you're likely to do it all over again when you re-encounter it at some in the future.
A simple comment here short-circuits (pun intended) that whole mental exercise.
If there are indeed only a few such instances within this repo, I'd be happy to amend this PR to cover those as well.
@stephentoubstephentoubAug 18, 2024

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.

Thanks. Personally, I still don't believe this is valuable. The only reason to use a | here is for the exact reason the comment would state.
@jww3jww3Aug 23, 2024

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

My guess is that most C# programmers aren't even aware that this is valid syntax for a boolean expression.
If you put yourself in their 👞👞 ...?
@am11am11
added

area-System.Runtime

needs-author-action
An issue or pull request that requires more info or actions from the author.
and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Jul 19, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Jul 31, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 31, 2024

Closed

3 tasks

This was referenced Jul 31, 2024

Open


Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 18, 2024

Closed

@jww3jww3
closed this
Aug 23, 2024

This was referenced Aug 23, 2024

Open


Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Sep 23, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+1 more reviewer

@huoyaoyuanhuoyaoyuanhuoyaoyuan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jww3@huoyaoyuan@PaulusParssinen@stephentoub@am11

, '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
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit - #102547
Closed
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Closed
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit#102547
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Conversation
@jww3
@jww3jww3
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Added the following comment to clarify the rationale for using | over || in Char::IsAsciiLetterOrDigit.
// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
May 22, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 22, 2024
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This can lead to the right-hand side of the expression being evaluated unnecessarily.
The JIT is able to optimize this case because the functions in two branches are trivial.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHjykDYBAgAbBgElcAQVxgAlvIAyMDBhhQAFGAAW2KAzABKBgF4AfA00BXeQDsMRzdoaEGABgSl3JuAwDk2P4mADymAQBe/gx+gf4A3Fw89EwCQiLiUgBCqgDuMDB22noGYGiGJQz49hJ2YKLWuPIAbjDluvpV2Ai19Y0tMCYWSdw29o4uftV2vQ1NraHhYw5O+N2z/a0xVTV1cwNGidQ8DCO8qYLCYpIycooqahoA8lAAIvIA5vIYxZ3GZpYpLIFMpVOotP83Nk8gUimUAu5/OV/ABOYJHE5nFL8S4ZG7A+5g55vT7fUi/UpDQG3EEPcHaEyEKG4HIYfKFbTIxHItGHGgAXyAA===
| and || provides the same assembly code, but || generates slightly longer IL.
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
This is intentional, see https://github.com/dotnet/runtime/pull/69318/files#r872595998
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Ah, I see. Thanks for clarifying! Maybe we should add a comment so that we don't keep revisiting this?
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
In my opinion it does warrant a comment so people don't need to pull out sharplab/disasmo to check why it's like this 😆
I think the rationale here is that the input is not considered to be predictable so we don't try have our stupidly smart modern branch predictors predict it(?). If one case is more common the checks could be ordered and the other version could be tiny bit faster (I think this sorta cmov data-dependecy issue vs a branch in a loop.. but kinda not as the current version doesn't even need conditional mov so it's even better like this..)
Also the IL is much nicer for the importer in the branchless version.
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024

Copy link


Copy Markdown

Member
Those operators evaluate the right-hand operand only if it's necessary.
In this case, branching itself has more overhead than evaluating either branch.

For short-circuiting, we usually care more about the side effect of evaluating a branch, including triggering NullReferenceException.
This can be a common sense of advanced optimization, but sometimes I can't notice it.
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Thanks! I'll convert this PR into just a PR to add an explanatory comment.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@jww3jww3
changed the title
Use short-circuiting || operator in Char::IsAsciiLetterOrDigitDocument the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
@jww3jww3
changed the title
Document the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitDocument the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
huoyaoyuan
huoyaoyuan
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
Outdated


Comment on lines

+277
 to 
+279

 // Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
 // It avoids a low-level branch instruction. (In other words, the overhead of branching
 // is greater than the cost of simply executing IsBetween.)
@huoyaoyuanhuoyaoyuanMay 22, 2024

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.

The comment looks too verbose for users who can understand it. The position is also questionable with xml doc.
I'd suggest a trailing comment on the same line, just saying "use arithmetic or to avoid branching" or similar.

This was referenced May 22, 2024

Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
May 23, 2024

Closed

@jww3
jww3
commented
May 23, 2024

Copy link


Copy Markdown

Author
Tests seem to be failing for an unrelated reason. (In the end, all I did was add a comment.) I don't seem to have permission to schedule a retry of the tests.
Any advice would be appreciated.
stephentoub
stephentoub
reviewed
Jun 23, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
 /// 'a' through 'z', inclusive, or '0' through '9', inclusive.
 /// </remarks>
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9');
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9'); // Using | here is an optimization (avoids branching).
@stephentoubstephentoubJun 23, 2024

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.

Thanks. There are a variety of places we use | like this, which is part of its raison d'etre. Why without a comment is there an assumption that it's a mistake and why is this particular use special so as to warrant a dedicated comment?
@jww3jww3Jul 31, 2024•

edited

Loading

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

When I do a quick search through this repo for " | ", I don't see many instances of | being used for boolean operations.
If there were hundreds (or even dozens) of similar usage patterns, I'd agree that it's a common-enough practice as to not warrant any clarification.
It seems, however, there are only a few.
The net effect is that the codebase exhibits these occasional inconsistencies. Mentally, when you encounter one, it gives you pause: "Wait. That doesn't look right. Why's that?"
Maybe you remember why it's that way. Maybe you have to jog your memory. Maybe you're stumped. (Most people would be stumped.)
Either way, you've wasted time contemplating it and you're likely to do it all over again when you re-encounter it at some in the future.
A simple comment here short-circuits (pun intended) that whole mental exercise.
If there are indeed only a few such instances within this repo, I'd be happy to amend this PR to cover those as well.
@stephentoubstephentoubAug 18, 2024

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.

Thanks. Personally, I still don't believe this is valuable. The only reason to use a | here is for the exact reason the comment would state.
@jww3jww3Aug 23, 2024

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

My guess is that most C# programmers aren't even aware that this is valid syntax for a boolean expression.
If you put yourself in their 👞👞 ...?
@am11am11
added

area-System.Runtime

needs-author-action
An issue or pull request that requires more info or actions from the author.
and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Jul 19, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Jul 31, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 31, 2024

Closed

3 tasks

This was referenced Jul 31, 2024

Open


Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 18, 2024

Closed

@jww3jww3
closed this
Aug 23, 2024

This was referenced Aug 23, 2024

Open


Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Sep 23, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+1 more reviewer

@huoyaoyuanhuoyaoyuanhuoyaoyuan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jww3@huoyaoyuan@PaulusParssinen@stephentoub@am11

, '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
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit - #102547
Closed
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Closed
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit#102547
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Conversation
@jww3
@jww3jww3
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Added the following comment to clarify the rationale for using | over || in Char::IsAsciiLetterOrDigit.
// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
May 22, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 22, 2024
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This can lead to the right-hand side of the expression being evaluated unnecessarily.
The JIT is able to optimize this case because the functions in two branches are trivial.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHjykDYBAgAbBgElcAQVxgAlvIAyMDBhhQAFGAAW2KAzABKBgF4AfA00BXeQDsMRzdoaEGABgSl3JuAwDk2P4mADymAQBe/gx+gf4A3Fw89EwCQiLiUgBCqgDuMDB22noGYGiGJQz49hJ2YKLWuPIAbjDluvpV2Ai19Y0tMCYWSdw29o4uftV2vQ1NraHhYw5O+N2z/a0xVTV1cwNGidQ8DCO8qYLCYpIycooqahoA8lAAIvIA5vIYxZ3GZpYpLIFMpVOotP83Nk8gUimUAu5/OV/ABOYJHE5nFL8S4ZG7A+5g55vT7fUi/UpDQG3EEPcHaEyEKG4HIYfKFbTIxHItGHGgAXyAA===
| and || provides the same assembly code, but || generates slightly longer IL.
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
This is intentional, see https://github.com/dotnet/runtime/pull/69318/files#r872595998
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Ah, I see. Thanks for clarifying! Maybe we should add a comment so that we don't keep revisiting this?
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
In my opinion it does warrant a comment so people don't need to pull out sharplab/disasmo to check why it's like this 😆
I think the rationale here is that the input is not considered to be predictable so we don't try have our stupidly smart modern branch predictors predict it(?). If one case is more common the checks could be ordered and the other version could be tiny bit faster (I think this sorta cmov data-dependecy issue vs a branch in a loop.. but kinda not as the current version doesn't even need conditional mov so it's even better like this..)
Also the IL is much nicer for the importer in the branchless version.
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024

Copy link


Copy Markdown

Member
Those operators evaluate the right-hand operand only if it's necessary.
In this case, branching itself has more overhead than evaluating either branch.

For short-circuiting, we usually care more about the side effect of evaluating a branch, including triggering NullReferenceException.
This can be a common sense of advanced optimization, but sometimes I can't notice it.
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Thanks! I'll convert this PR into just a PR to add an explanatory comment.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@jww3jww3
changed the title
Use short-circuiting || operator in Char::IsAsciiLetterOrDigitDocument the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
@jww3jww3
changed the title
Document the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitDocument the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
huoyaoyuan
huoyaoyuan
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
Outdated


Comment on lines

+277
 to 
+279

 // Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
 // It avoids a low-level branch instruction. (In other words, the overhead of branching
 // is greater than the cost of simply executing IsBetween.)
@huoyaoyuanhuoyaoyuanMay 22, 2024

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.

The comment looks too verbose for users who can understand it. The position is also questionable with xml doc.
I'd suggest a trailing comment on the same line, just saying "use arithmetic or to avoid branching" or similar.

This was referenced May 22, 2024

Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
May 23, 2024

Closed

@jww3
jww3
commented
May 23, 2024

Copy link


Copy Markdown

Author
Tests seem to be failing for an unrelated reason. (In the end, all I did was add a comment.) I don't seem to have permission to schedule a retry of the tests.
Any advice would be appreciated.
stephentoub
stephentoub
reviewed
Jun 23, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
 /// 'a' through 'z', inclusive, or '0' through '9', inclusive.
 /// </remarks>
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9');
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9'); // Using | here is an optimization (avoids branching).
@stephentoubstephentoubJun 23, 2024

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.

Thanks. There are a variety of places we use | like this, which is part of its raison d'etre. Why without a comment is there an assumption that it's a mistake and why is this particular use special so as to warrant a dedicated comment?
@jww3jww3Jul 31, 2024•

edited

Loading

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

When I do a quick search through this repo for " | ", I don't see many instances of | being used for boolean operations.
If there were hundreds (or even dozens) of similar usage patterns, I'd agree that it's a common-enough practice as to not warrant any clarification.
It seems, however, there are only a few.
The net effect is that the codebase exhibits these occasional inconsistencies. Mentally, when you encounter one, it gives you pause: "Wait. That doesn't look right. Why's that?"
Maybe you remember why it's that way. Maybe you have to jog your memory. Maybe you're stumped. (Most people would be stumped.)
Either way, you've wasted time contemplating it and you're likely to do it all over again when you re-encounter it at some in the future.
A simple comment here short-circuits (pun intended) that whole mental exercise.
If there are indeed only a few such instances within this repo, I'd be happy to amend this PR to cover those as well.
@stephentoubstephentoubAug 18, 2024

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.

Thanks. Personally, I still don't believe this is valuable. The only reason to use a | here is for the exact reason the comment would state.
@jww3jww3Aug 23, 2024

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

My guess is that most C# programmers aren't even aware that this is valid syntax for a boolean expression.
If you put yourself in their 👞👞 ...?
@am11am11
added

area-System.Runtime

needs-author-action
An issue or pull request that requires more info or actions from the author.
and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Jul 19, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Jul 31, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 31, 2024

Closed

3 tasks

This was referenced Jul 31, 2024

Open


Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 18, 2024

Closed

@jww3jww3
closed this
Aug 23, 2024

This was referenced Aug 23, 2024

Open


Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Sep 23, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+1 more reviewer

@huoyaoyuanhuoyaoyuanhuoyaoyuan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jww3@huoyaoyuan@PaulusParssinen@stephentoub@am11

, '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
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit - #102547
Closed
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Closed
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit#102547
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Conversation
@jww3
@jww3jww3
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Added the following comment to clarify the rationale for using | over || in Char::IsAsciiLetterOrDigit.
// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
May 22, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 22, 2024
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This can lead to the right-hand side of the expression being evaluated unnecessarily.
The JIT is able to optimize this case because the functions in two branches are trivial.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHjykDYBAgAbBgElcAQVxgAlvIAyMDBhhQAFGAAW2KAzABKBgF4AfA00BXeQDsMRzdoaEGABgSl3JuAwDk2P4mADymAQBe/gx+gf4A3Fw89EwCQiLiUgBCqgDuMDB22noGYGiGJQz49hJ2YKLWuPIAbjDluvpV2Ai19Y0tMCYWSdw29o4uftV2vQ1NraHhYw5O+N2z/a0xVTV1cwNGidQ8DCO8qYLCYpIycooqahoA8lAAIvIA5vIYxZ3GZpYpLIFMpVOotP83Nk8gUimUAu5/OV/ABOYJHE5nFL8S4ZG7A+5g55vT7fUi/UpDQG3EEPcHaEyEKG4HIYfKFbTIxHItGHGgAXyAA===
| and || provides the same assembly code, but || generates slightly longer IL.
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
This is intentional, see https://github.com/dotnet/runtime/pull/69318/files#r872595998
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Ah, I see. Thanks for clarifying! Maybe we should add a comment so that we don't keep revisiting this?
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
In my opinion it does warrant a comment so people don't need to pull out sharplab/disasmo to check why it's like this 😆
I think the rationale here is that the input is not considered to be predictable so we don't try have our stupidly smart modern branch predictors predict it(?). If one case is more common the checks could be ordered and the other version could be tiny bit faster (I think this sorta cmov data-dependecy issue vs a branch in a loop.. but kinda not as the current version doesn't even need conditional mov so it's even better like this..)
Also the IL is much nicer for the importer in the branchless version.
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024

Copy link


Copy Markdown

Member
Those operators evaluate the right-hand operand only if it's necessary.
In this case, branching itself has more overhead than evaluating either branch.

For short-circuiting, we usually care more about the side effect of evaluating a branch, including triggering NullReferenceException.
This can be a common sense of advanced optimization, but sometimes I can't notice it.
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Thanks! I'll convert this PR into just a PR to add an explanatory comment.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@jww3jww3
changed the title
Use short-circuiting || operator in Char::IsAsciiLetterOrDigitDocument the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
@jww3jww3
changed the title
Document the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitDocument the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
huoyaoyuan
huoyaoyuan
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
Outdated


Comment on lines

+277
 to 
+279

 // Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
 // It avoids a low-level branch instruction. (In other words, the overhead of branching
 // is greater than the cost of simply executing IsBetween.)
@huoyaoyuanhuoyaoyuanMay 22, 2024

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.

The comment looks too verbose for users who can understand it. The position is also questionable with xml doc.
I'd suggest a trailing comment on the same line, just saying "use arithmetic or to avoid branching" or similar.

This was referenced May 22, 2024

Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
May 23, 2024

Closed

@jww3
jww3
commented
May 23, 2024

Copy link


Copy Markdown

Author
Tests seem to be failing for an unrelated reason. (In the end, all I did was add a comment.) I don't seem to have permission to schedule a retry of the tests.
Any advice would be appreciated.
stephentoub
stephentoub
reviewed
Jun 23, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
 /// 'a' through 'z', inclusive, or '0' through '9', inclusive.
 /// </remarks>
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9');
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9'); // Using | here is an optimization (avoids branching).
@stephentoubstephentoubJun 23, 2024

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.

Thanks. There are a variety of places we use | like this, which is part of its raison d'etre. Why without a comment is there an assumption that it's a mistake and why is this particular use special so as to warrant a dedicated comment?
@jww3jww3Jul 31, 2024•

edited

Loading

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

When I do a quick search through this repo for " | ", I don't see many instances of | being used for boolean operations.
If there were hundreds (or even dozens) of similar usage patterns, I'd agree that it's a common-enough practice as to not warrant any clarification.
It seems, however, there are only a few.
The net effect is that the codebase exhibits these occasional inconsistencies. Mentally, when you encounter one, it gives you pause: "Wait. That doesn't look right. Why's that?"
Maybe you remember why it's that way. Maybe you have to jog your memory. Maybe you're stumped. (Most people would be stumped.)
Either way, you've wasted time contemplating it and you're likely to do it all over again when you re-encounter it at some in the future.
A simple comment here short-circuits (pun intended) that whole mental exercise.
If there are indeed only a few such instances within this repo, I'd be happy to amend this PR to cover those as well.
@stephentoubstephentoubAug 18, 2024

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.

Thanks. Personally, I still don't believe this is valuable. The only reason to use a | here is for the exact reason the comment would state.
@jww3jww3Aug 23, 2024

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

My guess is that most C# programmers aren't even aware that this is valid syntax for a boolean expression.
If you put yourself in their 👞👞 ...?
@am11am11
added

area-System.Runtime

needs-author-action
An issue or pull request that requires more info or actions from the author.
and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Jul 19, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Jul 31, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 31, 2024

Closed

3 tasks

This was referenced Jul 31, 2024

Open


Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 18, 2024

Closed

@jww3jww3
closed this
Aug 23, 2024

This was referenced Aug 23, 2024

Open


Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Sep 23, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+1 more reviewer

@huoyaoyuanhuoyaoyuanhuoyaoyuan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jww3@huoyaoyuan@PaulusParssinen@stephentoub@am11

, '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
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit - #102547
Closed
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Closed
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit#102547
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Conversation
@jww3
@jww3jww3
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Added the following comment to clarify the rationale for using | over || in Char::IsAsciiLetterOrDigit.
// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
May 22, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 22, 2024
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This can lead to the right-hand side of the expression being evaluated unnecessarily.
The JIT is able to optimize this case because the functions in two branches are trivial.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHjykDYBAgAbBgElcAQVxgAlvIAyMDBhhQAFGAAW2KAzABKBgF4AfA00BXeQDsMRzdoaEGABgSl3JuAwDk2P4mADymAQBe/gx+gf4A3Fw89EwCQiLiUgBCqgDuMDB22noGYGiGJQz49hJ2YKLWuPIAbjDluvpV2Ai19Y0tMCYWSdw29o4uftV2vQ1NraHhYw5O+N2z/a0xVTV1cwNGidQ8DCO8qYLCYpIycooqahoA8lAAIvIA5vIYxZ3GZpYpLIFMpVOotP83Nk8gUimUAu5/OV/ABOYJHE5nFL8S4ZG7A+5g55vT7fUi/UpDQG3EEPcHaEyEKG4HIYfKFbTIxHItGHGgAXyAA===
| and || provides the same assembly code, but || generates slightly longer IL.
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
This is intentional, see https://github.com/dotnet/runtime/pull/69318/files#r872595998
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Ah, I see. Thanks for clarifying! Maybe we should add a comment so that we don't keep revisiting this?
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
In my opinion it does warrant a comment so people don't need to pull out sharplab/disasmo to check why it's like this 😆
I think the rationale here is that the input is not considered to be predictable so we don't try have our stupidly smart modern branch predictors predict it(?). If one case is more common the checks could be ordered and the other version could be tiny bit faster (I think this sorta cmov data-dependecy issue vs a branch in a loop.. but kinda not as the current version doesn't even need conditional mov so it's even better like this..)
Also the IL is much nicer for the importer in the branchless version.
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024

Copy link


Copy Markdown

Member
Those operators evaluate the right-hand operand only if it's necessary.
In this case, branching itself has more overhead than evaluating either branch.

For short-circuiting, we usually care more about the side effect of evaluating a branch, including triggering NullReferenceException.
This can be a common sense of advanced optimization, but sometimes I can't notice it.
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Thanks! I'll convert this PR into just a PR to add an explanatory comment.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@jww3jww3
changed the title
Use short-circuiting || operator in Char::IsAsciiLetterOrDigitDocument the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
@jww3jww3
changed the title
Document the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitDocument the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
huoyaoyuan
huoyaoyuan
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
Outdated


Comment on lines

+277
 to 
+279

 // Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
 // It avoids a low-level branch instruction. (In other words, the overhead of branching
 // is greater than the cost of simply executing IsBetween.)
@huoyaoyuanhuoyaoyuanMay 22, 2024

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.

The comment looks too verbose for users who can understand it. The position is also questionable with xml doc.
I'd suggest a trailing comment on the same line, just saying "use arithmetic or to avoid branching" or similar.

This was referenced May 22, 2024

Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
May 23, 2024

Closed

@jww3
jww3
commented
May 23, 2024

Copy link


Copy Markdown

Author
Tests seem to be failing for an unrelated reason. (In the end, all I did was add a comment.) I don't seem to have permission to schedule a retry of the tests.
Any advice would be appreciated.
stephentoub
stephentoub
reviewed
Jun 23, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
 /// 'a' through 'z', inclusive, or '0' through '9', inclusive.
 /// </remarks>
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9');
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9'); // Using | here is an optimization (avoids branching).
@stephentoubstephentoubJun 23, 2024

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.

Thanks. There are a variety of places we use | like this, which is part of its raison d'etre. Why without a comment is there an assumption that it's a mistake and why is this particular use special so as to warrant a dedicated comment?
@jww3jww3Jul 31, 2024•

edited

Loading

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

When I do a quick search through this repo for " | ", I don't see many instances of | being used for boolean operations.
If there were hundreds (or even dozens) of similar usage patterns, I'd agree that it's a common-enough practice as to not warrant any clarification.
It seems, however, there are only a few.
The net effect is that the codebase exhibits these occasional inconsistencies. Mentally, when you encounter one, it gives you pause: "Wait. That doesn't look right. Why's that?"
Maybe you remember why it's that way. Maybe you have to jog your memory. Maybe you're stumped. (Most people would be stumped.)
Either way, you've wasted time contemplating it and you're likely to do it all over again when you re-encounter it at some in the future.
A simple comment here short-circuits (pun intended) that whole mental exercise.
If there are indeed only a few such instances within this repo, I'd be happy to amend this PR to cover those as well.
@stephentoubstephentoubAug 18, 2024

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.

Thanks. Personally, I still don't believe this is valuable. The only reason to use a | here is for the exact reason the comment would state.
@jww3jww3Aug 23, 2024

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

My guess is that most C# programmers aren't even aware that this is valid syntax for a boolean expression.
If you put yourself in their 👞👞 ...?
@am11am11
added

area-System.Runtime

needs-author-action
An issue or pull request that requires more info or actions from the author.
and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Jul 19, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Jul 31, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 31, 2024

Closed

3 tasks

This was referenced Jul 31, 2024

Open


Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 18, 2024

Closed

@jww3jww3
closed this
Aug 23, 2024

This was referenced Aug 23, 2024

Open


Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Sep 23, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+1 more reviewer

@huoyaoyuanhuoyaoyuanhuoyaoyuan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jww3@huoyaoyuan@PaulusParssinen@stephentoub@am11

, '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
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit - #102547
Closed
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Closed
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit#102547
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Conversation
@jww3
@jww3jww3
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Added the following comment to clarify the rationale for using | over || in Char::IsAsciiLetterOrDigit.
// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
May 22, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 22, 2024
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This can lead to the right-hand side of the expression being evaluated unnecessarily.
The JIT is able to optimize this case because the functions in two branches are trivial.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHjykDYBAgAbBgElcAQVxgAlvIAyMDBhhQAFGAAW2KAzABKBgF4AfA00BXeQDsMRzdoaEGABgSl3JuAwDk2P4mADymAQBe/gx+gf4A3Fw89EwCQiLiUgBCqgDuMDB22noGYGiGJQz49hJ2YKLWuPIAbjDluvpV2Ai19Y0tMCYWSdw29o4uftV2vQ1NraHhYw5O+N2z/a0xVTV1cwNGidQ8DCO8qYLCYpIycooqahoA8lAAIvIA5vIYxZ3GZpYpLIFMpVOotP83Nk8gUimUAu5/OV/ABOYJHE5nFL8S4ZG7A+5g55vT7fUi/UpDQG3EEPcHaEyEKG4HIYfKFbTIxHItGHGgAXyAA===
| and || provides the same assembly code, but || generates slightly longer IL.
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
This is intentional, see https://github.com/dotnet/runtime/pull/69318/files#r872595998
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Ah, I see. Thanks for clarifying! Maybe we should add a comment so that we don't keep revisiting this?
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
In my opinion it does warrant a comment so people don't need to pull out sharplab/disasmo to check why it's like this 😆
I think the rationale here is that the input is not considered to be predictable so we don't try have our stupidly smart modern branch predictors predict it(?). If one case is more common the checks could be ordered and the other version could be tiny bit faster (I think this sorta cmov data-dependecy issue vs a branch in a loop.. but kinda not as the current version doesn't even need conditional mov so it's even better like this..)
Also the IL is much nicer for the importer in the branchless version.
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024

Copy link


Copy Markdown

Member
Those operators evaluate the right-hand operand only if it's necessary.
In this case, branching itself has more overhead than evaluating either branch.

For short-circuiting, we usually care more about the side effect of evaluating a branch, including triggering NullReferenceException.
This can be a common sense of advanced optimization, but sometimes I can't notice it.
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Thanks! I'll convert this PR into just a PR to add an explanatory comment.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@jww3jww3
changed the title
Use short-circuiting || operator in Char::IsAsciiLetterOrDigitDocument the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
@jww3jww3
changed the title
Document the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitDocument the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
huoyaoyuan
huoyaoyuan
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
Outdated


Comment on lines

+277
 to 
+279

 // Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
 // It avoids a low-level branch instruction. (In other words, the overhead of branching
 // is greater than the cost of simply executing IsBetween.)
@huoyaoyuanhuoyaoyuanMay 22, 2024

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.

The comment looks too verbose for users who can understand it. The position is also questionable with xml doc.
I'd suggest a trailing comment on the same line, just saying "use arithmetic or to avoid branching" or similar.

This was referenced May 22, 2024

Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
May 23, 2024

Closed

@jww3
jww3
commented
May 23, 2024

Copy link


Copy Markdown

Author
Tests seem to be failing for an unrelated reason. (In the end, all I did was add a comment.) I don't seem to have permission to schedule a retry of the tests.
Any advice would be appreciated.
stephentoub
stephentoub
reviewed
Jun 23, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
 /// 'a' through 'z', inclusive, or '0' through '9', inclusive.
 /// </remarks>
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9');
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9'); // Using | here is an optimization (avoids branching).
@stephentoubstephentoubJun 23, 2024

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.

Thanks. There are a variety of places we use | like this, which is part of its raison d'etre. Why without a comment is there an assumption that it's a mistake and why is this particular use special so as to warrant a dedicated comment?
@jww3jww3Jul 31, 2024•

edited

Loading

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

When I do a quick search through this repo for " | ", I don't see many instances of | being used for boolean operations.
If there were hundreds (or even dozens) of similar usage patterns, I'd agree that it's a common-enough practice as to not warrant any clarification.
It seems, however, there are only a few.
The net effect is that the codebase exhibits these occasional inconsistencies. Mentally, when you encounter one, it gives you pause: "Wait. That doesn't look right. Why's that?"
Maybe you remember why it's that way. Maybe you have to jog your memory. Maybe you're stumped. (Most people would be stumped.)
Either way, you've wasted time contemplating it and you're likely to do it all over again when you re-encounter it at some in the future.
A simple comment here short-circuits (pun intended) that whole mental exercise.
If there are indeed only a few such instances within this repo, I'd be happy to amend this PR to cover those as well.
@stephentoubstephentoubAug 18, 2024

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.

Thanks. Personally, I still don't believe this is valuable. The only reason to use a | here is for the exact reason the comment would state.
@jww3jww3Aug 23, 2024

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

My guess is that most C# programmers aren't even aware that this is valid syntax for a boolean expression.
If you put yourself in their 👞👞 ...?
@am11am11
added

area-System.Runtime

needs-author-action
An issue or pull request that requires more info or actions from the author.
and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Jul 19, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Jul 31, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 31, 2024

Closed

3 tasks

This was referenced Jul 31, 2024

Open


Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 18, 2024

Closed

@jww3jww3
closed this
Aug 23, 2024

This was referenced Aug 23, 2024

Open


Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Sep 23, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+1 more reviewer

@huoyaoyuanhuoyaoyuanhuoyaoyuan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jww3@huoyaoyuan@PaulusParssinen@stephentoub@am11

, '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
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit - #102547
Closed
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Closed
Document the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigit#102547
jww3 wants to merge 10 commits into
dotnet:mainfrom 
jww3:patch-1
Conversation
@jww3
@jww3jww3
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Added the following comment to clarify the rationale for using | over || in Char::IsAsciiLetterOrDigit.
// Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
// It avoids a low-level branch instruction. (In other words, the overhead of branching
// is greater than the cost of simply executing IsBetween.)

@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
May 22, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 22, 2024
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This can lead to the right-hand side of the expression being evaluated unnecessarily.
The JIT is able to optimize this case because the functions in two branches are trivial.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunHjykDYBAgAbBgElcAQVxgAlvIAyMDBhhQAFGAAW2KAzABKBgF4AfA00BXeQDsMRzdoaEGABgSl3JuAwDk2P4mADymAQBe/gx+gf4A3Fw89EwCQiLiUgBCqgDuMDB22noGYGiGJQz49hJ2YKLWuPIAbjDluvpV2Ai19Y0tMCYWSdw29o4uftV2vQ1NraHhYw5O+N2z/a0xVTV1cwNGidQ8DCO8qYLCYpIycooqahoA8lAAIvIA5vIYxZ3GZpYpLIFMpVOotP83Nk8gUimUAu5/OV/ABOYJHE5nFL8S4ZG7A+5g55vT7fUi/UpDQG3EEPcHaEyEKG4HIYfKFbTIxHItGHGgAXyAA===
| and || provides the same assembly code, but || generates slightly longer IL.
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024

Copy link


Copy Markdown

Contributor
This is intentional, see https://github.com/dotnet/runtime/pull/69318/files#r872595998
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Ah, I see. Thanks for clarifying! Maybe we should add a comment so that we don't keep revisiting this?
@PaulusParssinen
PaulusParssinen
commented
May 22, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
In my opinion it does warrant a comment so people don't need to pull out sharplab/disasmo to check why it's like this 😆
I think the rationale here is that the input is not considered to be predictable so we don't try have our stupidly smart modern branch predictors predict it(?). If one case is more common the checks could be ordered and the other version could be tiny bit faster (I think this sorta cmov data-dependecy issue vs a branch in a loop.. but kinda not as the current version doesn't even need conditional mov so it's even better like this..)
Also the IL is much nicer for the importer in the branchless version.
@huoyaoyuan
huoyaoyuan
commented
May 22, 2024

Copy link


Copy Markdown

Member
Those operators evaluate the right-hand operand only if it's necessary.
In this case, branching itself has more overhead than evaluating either branch.

For short-circuiting, we usually care more about the side effect of evaluating a branch, including triggering NullReferenceException.
This can be a common sense of advanced optimization, but sometimes I can't notice it.
@jww3
jww3
commented
May 22, 2024

Copy link


Copy Markdown

Author
Thanks! I'll convert this PR into just a PR to add an explanatory comment.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@jww3jww3
changed the title
Use short-circuiting || operator in Char::IsAsciiLetterOrDigitDocument the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
@jww3jww3
changed the title
Document the use non-short-circuiting | operator in Char::IsAsciiLetterOrDigitDocument the use of non-short-circuiting | operator in Char::IsAsciiLetterOrDigitMay 22, 2024
huoyaoyuan
huoyaoyuan
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
Outdated


Comment on lines

+277
 to 
+279

 // Use of the non-short-circuiting logical OR operator (|) here is an intentional optimization.
 // It avoids a low-level branch instruction. (In other words, the overhead of branching
 // is greater than the cost of simply executing IsBetween.)
@huoyaoyuanhuoyaoyuanMay 22, 2024

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.

The comment looks too verbose for users who can understand it. The position is also questionable with xml doc.
I'd suggest a trailing comment on the same line, just saying "use arithmetic or to avoid branching" or similar.

This was referenced May 22, 2024

Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
May 23, 2024

Closed

@jww3
jww3
commented
May 23, 2024

Copy link


Copy Markdown

Author
Tests seem to be failing for an unrelated reason. (In the end, all I did was add a comment.) I don't seem to have permission to schedule a retry of the tests.
Any advice would be appreciated.
stephentoub
stephentoub
reviewed
Jun 23, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Char.cs
 /// 'a' through 'z', inclusive, or '0' through '9', inclusive.
 /// </remarks>
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9');
 public static bool IsAsciiLetterOrDigit(char c) => IsAsciiLetter(c) | IsBetween(c, '0', '9'); // Using | here is an optimization (avoids branching).
@stephentoubstephentoubJun 23, 2024

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.

Thanks. There are a variety of places we use | like this, which is part of its raison d'etre. Why without a comment is there an assumption that it's a mistake and why is this particular use special so as to warrant a dedicated comment?
@jww3jww3Jul 31, 2024•

edited

Loading

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

When I do a quick search through this repo for " | ", I don't see many instances of | being used for boolean operations.
If there were hundreds (or even dozens) of similar usage patterns, I'd agree that it's a common-enough practice as to not warrant any clarification.
It seems, however, there are only a few.
The net effect is that the codebase exhibits these occasional inconsistencies. Mentally, when you encounter one, it gives you pause: "Wait. That doesn't look right. Why's that?"
Maybe you remember why it's that way. Maybe you have to jog your memory. Maybe you're stumped. (Most people would be stumped.)
Either way, you've wasted time contemplating it and you're likely to do it all over again when you re-encounter it at some in the future.
A simple comment here short-circuits (pun intended) that whole mental exercise.
If there are indeed only a few such instances within this repo, I'd be happy to amend this PR to cover those as well.
@stephentoubstephentoubAug 18, 2024

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.

Thanks. Personally, I still don't believe this is valuable. The only reason to use a | here is for the exact reason the comment would state.
@jww3jww3Aug 23, 2024

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

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

My guess is that most C# programmers aren't even aware that this is valid syntax for a boolean expression.
If you put yourself in their 👞👞 ...?
@am11am11
added

area-System.Runtime

needs-author-action
An issue or pull request that requires more info or actions from the author.
and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Jul 19, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Jul 31, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 31, 2024

Closed

3 tasks

This was referenced Jul 31, 2024

Open


Closed


Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 18, 2024

Closed

@jww3jww3
closed this
Aug 23, 2024

This was referenced Aug 23, 2024

Open


Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Sep 23, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+1 more reviewer

@huoyaoyuanhuoyaoyuanhuoyaoyuan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jww3@huoyaoyuan@PaulusParssinen@stephentoub@am11