Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Workaround to remove unnecessary bounds checks when using {ReadOnly}Span.IsEmpty#19640

Merged
ahsonkhan merged 1 commit intodotnet:masterfrom
ahsonkhan:RemoveBoundsCheck
Aug 24, 2018
Merged

Workaround to remove unnecessary bounds checks when using {ReadOnly}Span.IsEmpty#19640
ahsonkhan merged 1 commit intodotnet:masterfrom
ahsonkhan:RemoveBoundsCheck

Conversation

@ahsonkhan
Copy link

@ahsonkhan ahsonkhan commented Aug 23, 2018

Workaround for https://github.com/dotnet/coreclr/issues/19620

public static bool IsFirstSpace(ReadOnlySpan<byte> buffer)
{
    if (buffer.IsEmpty || buffer[0] != ' ')
        return false;
    return true;
}

Disassembly for before/after:
image

cc @AndyAyersMS, @benaadams, @GrabYourPitchforks, @mikedn

@ahsonkhan ahsonkhan self-assigned this Aug 23, 2018
ahsonkhan
ahsonkhan commented Aug 23, 2018
View reviewed changes
src/System.Private.CoreLib/shared/System/ReadOnlySpan.cs
@@ -40,7 +40,8 @@ public bool IsEmpty
[NonVersionable]
Copy link
Author

@ahsonkhan ahsonkhan Aug 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jkotas, can we make such changes even if this property is marked as NonVersionable?

It was marked as such here: #13931

Copy link
Member

@jkotas jkotas Aug 23, 2018

Choose a reason for hiding this comment

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

This is ok. It is not changing behavior (for valid lengths). We have done the same change for IsNullOrEmpty that is also marked NonVersionable.

@ahsonkhan ahsonkhan merged commit 3ce59b1 into dotnet:master Aug 24, 2018
@ahsonkhan ahsonkhan deleted the RemoveBoundsCheck branch August 24, 2018 02:11
@ArtBlnd
Copy link

ArtBlnd commented Aug 24, 2018

Um... I don't think this is a real solution for removing bound checks.

@ArtBlnd
Copy link

ArtBlnd commented Aug 24, 2018

As @mikedn saied

This has little to do with condition reversing. Sure, that may happen to help, but the fundamental issue is that the JIT's range check elimination capabilities are kind of limited. It's designed to recognize certain very common patterns (for (i = 0; i < a.Length; i++) a[i]…) and tends to fail in pretty much any other circumstances, even relatively trivial ones.

Seems more reasonable implements more cases to estimates bound checks :>

@mikedn
Copy link

mikedn commented Aug 24, 2018

Seems more reasonable implements more cases to estimates bound checks :>

Could be, I haven't checked yet. But usually it turns out that doing that comes with some trade-offs in the current implementation. And then it's simpler to just change the C# code here and there, for now.

jkotas referenced this pull request in dotnet/corefx Aug 24, 2018
@ahsonkhan @jkotas
Workaround to remove unnecessary bounds checks when using {ReadOnly}S…
95fa11b 
…pan.IsEmpty (#19640)

Signed-off-by: dotnet-bot <[email protected]>
@ahsonkhan
Copy link
Author

ahsonkhan commented Aug 24, 2018 •
edited
Loading

Um... I don't think this is a real solution for removing bound checks.

Seems more reasonable implements more cases to estimates bound checks

Yes, this is only a workaround for one of the more common uses, until the JIT is able to resolve the issue (if it can).

xtqqczze added a commit to xtqqczze/dotnet-runtime that referenced this pull request Mar 18, 2022
@xtqqczze
Revert "Workaround to remove unnecessary bounds checks when using {Re…
f1449b2 
…adOnly}Span.IsEmpty (dotnet/coreclr#19640)"

This reverts commit 82bd67f.
AaronRobinsonMSFT added a commit to dotnet/runtime that referenced this pull request Mar 29, 2022
@xtqqczze @AaronRobinsonMSFT @deeprobin
Remove IsEmpty workaround in Span and ReadOnlySpan (#66865)
1e2485f 
* Revert "Workaround to remove unnecessary bounds checks when using {ReadOnly}Span.IsEmpty (dotnet/coreclr#19640)"

This reverts commit 82bd67f.

* Update doc comments

* Apply suggestions from code review

Co-authored-by: Robin Lindner <[email protected]>

* Apply suggestions from code review

Co-authored-by: Robin Lindner <[email protected]>

Co-authored-by: Aaron Robinson <[email protected]>
Co-authored-by: Robin Lindner <[email protected]>
Co-authored-by: Robin Lindner <[email protected]>
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
@xtqqczze @AaronRobinsonMSFT
@deeprobin @radekdoulik
Remove IsEmpty workaround in Span and ReadOnlySpan (dotnet#66865)
3e871a0 
* Revert "Workaround to remove unnecessary bounds checks when using {ReadOnly}Span.IsEmpty (dotnet/coreclr#19640)"

This reverts commit 82bd67f.

* Update doc comments

* Apply suggestions from code review

Co-authored-by: Robin Lindner <[email protected]>

* Apply suggestions from code review

Co-authored-by: Robin Lindner <[email protected]>

Co-authored-by: Aaron Robinson <[email protected]>
Co-authored-by: Robin Lindner <[email protected]>
Co-authored-by: Robin Lindner <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jkotas jkotas jkotas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ahsonkhan ahsonkhan

Labels

area-System.Memory optimization tenet-performance Performance related issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ahsonkhan @ArtBlnd @mikedn @jkotas