Skip to content

Remove IsEmpty workaround in Span and ReadOnlySpan#66865

Merged
AaronRobinsonMSFT merged 4 commits intodotnet:mainfrom
xtqqczze:GH64514
Mar 29, 2022
Merged

Remove IsEmpty workaround in Span and ReadOnlySpan#66865
AaronRobinsonMSFT merged 4 commits intodotnet:mainfrom
xtqqczze:GH64514

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Mar 18, 2022

Fix #64514

/cc @deeprobin

@ghost ghost added area-System.Runtime community-contribution Indicates that the PR has been added by a community member labels Mar 18, 2022
@ghost
Copy link

ghost commented Mar 18, 2022

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

Issue Details

Fix #64514

/cc @deeprobin

Author: xtqqczze
Assignees: -
Labels:

area-System.Runtime
Milestone: -

deeprobin
deeprobin suggested changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@deeprobin deeprobin left a comment

Choose a reason for hiding this comment

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

Hey, I took a look at the pull request.

I still have a small change suggestion to the documentation comments so that this corresponds to the already existing scheme. In my opinion there is nothing against merging this then.

Generally, people wait until an issue is no longer untriaged before writing code.

Since this is a small change, it probably wouldn't be too bad if the area owner agrees.

Have you also verified if the JIT CodeGen is the same?
A Godbolt example would then make this easier for the reviewer.

Thank you very much for your efforts. More contributions are welcome :).

deeprobin
deeprobin reviewed Mar 28, 2022
View reviewed changes
deeprobin
deeprobin reviewed Mar 28, 2022
View reviewed changes
@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Mar 28, 2022

Results from a jit-diff

Found 2 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 3600063
Total bytes of diff: 3600030
Total bytes of delta: -33 (-0.00 % of base)
Total relative delta: -0.07
    diff is an improvement.
    relative diff is an improvement.


Top file improvements (bytes):
         -33 : System.Private.CoreLib.dasm (-0.00% of base)

1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged.

Top method regressions (bytes):
          15 ( 2.45% of base) : System.Private.CoreLib.dasm - System.String:JoinCore(System.ReadOnlySpan`1[System.Char],System.ReadOnlySpan`1[System.String]):System.String

Top method improvements (bytes):
         -24 (-4.73% of base) : System.Private.CoreLib.dasm - System.Globalization.CompareInfo:IndexOf(System.ReadOnlySpan`1[System.Char],System.ReadOnlySpan`1[System.Char],int):int:this
         -24 (-4.51% of base) : System.Private.CoreLib.dasm - System.Globalization.CompareInfo:LastIndexOf(System.ReadOnlySpan`1[System.Char],System.ReadOnlySpan`1[System.Char],int):int:this

Top method regressions (percentages):
          15 ( 2.45% of base) : System.Private.CoreLib.dasm - System.String:JoinCore(System.ReadOnlySpan`1[System.Char],System.ReadOnlySpan`1[System.String]):System.String

Top method improvements (percentages):
         -24 (-4.73% of base) : System.Private.CoreLib.dasm - System.Globalization.CompareInfo:IndexOf(System.ReadOnlySpan`1[System.Char],System.ReadOnlySpan`1[System.Char],int):int:this
         -24 (-4.51% of base) : System.Private.CoreLib.dasm - System.Globalization.CompareInfo:LastIndexOf(System.ReadOnlySpan`1[System.Char],System.ReadOnlySpan`1[System.Char],int):int:this

3 total methods with Code Size differences (2 improved, 1 regressed), 25225 unchanged.

@EgorBo Any concerns with this change?

@EgorBo EgorBo mentioned this pull request Mar 28, 2022
Closed
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 1e2485f into dotnet:main Mar 29, 2022
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]>
@ghost ghost locked as resolved and limited conversation to collaborators Apr 28, 2022
@xtqqczze xtqqczze deleted the GH64514 branch May 12, 2022 17:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

+1 more reviewer

@deeprobin deeprobin deeprobin requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Runtime community-contribution Indicates 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.

Remove IsEmpty workaround in Span and ReadOnlySpan

3 participants

@xtqqczze @AaronRobinsonMSFT @deeprobin