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

Improve perf for Index based span indexers #21196

Merged
AndyAyersMS merged 2 commits into from
Nov 27, 2018
Merged

Improve perf for Index based span indexers #21196

AndyAyersMS merged 2 commits into from
Nov 27, 2018

Conversation

@AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Nov 26, 2018

First, evaluate the actual index before invoking the underlying int indexer,
so that the jit doesn't think the span is address taken.

Second, improve the jit's ability to remove some redundant comparisions like
those that arise in computing the actual index.

@AndyAyersMS
Improve perf for Index based span indexers
410e98c 
First, evaluate the actual index before invoking the underlying int indexer,
so that the jit doesn't think the span is address taken.

Second, improve the jit's ability to remove some redundant comparisions like
those that arise in computing the actual index.
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Nov 26, 2018

@briansull PTAL
cc @dotnet/jit-contrib @tarekgh

jit-diffs shows impact mainly to the new Index based operations on spans:

Total bytes of diff: -1099 (0.00% of base)
    diff is an improvement.
Top file improvements by size (bytes):
        -873 : System.Private.CoreLib.dasm (-0.02% of base)
         -46 : System.Data.Common.dasm (0.00% of base)
         -32 : System.Private.Xml.dasm (0.00% of base)
         -26 : Microsoft.CodeAnalysis.dasm (0.00% of base)
         -18 : Microsoft.VisualBasic.dasm (-0.01% of base)
17 total files with size differences (17 improved, 0 regressed), 112 unchanged.
Top method improvements by size (bytes):
        -234 (-28.23% of base) : System.Private.CoreLib.dasm - ReadOnlySpan`1:get_Item(struct):struct:this (5 methods)
        -234 (-28.23% of base) : System.Private.CoreLib.dasm - Span`1:get_Item(struct):struct:this (5 methods)
        -118 (-34.20% of base) : System.Private.CoreLib.dasm - ReadOnlySpan`1:get_Item(struct):byref:this (5 methods)
        -102 (-27.72% of base) : System.Private.CoreLib.dasm - Span`1:get_Item(struct):byref:this (5 methods)
         -80 (-31.87% of base) : System.Private.CoreLib.dasm - EventPipePayloadDecoder:ReadUnalignedGuid(byref):struct
Top method improvements by size (percentage):
        -118 (-34.20% of base) : System.Private.CoreLib.dasm - ReadOnlySpan`1:get_Item(struct):byref:this (5 methods)
         -80 (-31.87% of base) : System.Private.CoreLib.dasm - EventPipePayloadDecoder:ReadUnalignedGuid(byref):struct
        -234 (-28.23% of base) : System.Private.CoreLib.dasm - ReadOnlySpan`1:get_Item(struct):struct:this (5 methods)
        -234 (-28.23% of base) : System.Private.CoreLib.dasm - Span`1:get_Item(struct):struct:this (5 methods)
        -102 (-27.72% of base) : System.Private.CoreLib.dasm - Span`1:get_Item(struct):byref:this (5 methods)
43 total methods with size differences (43 improved, 0 regressed), 124016 unchanged.

Performance on a simple benchmark (see dotnet/corefx#33376) improved from ~80ms to ~57ms, matching "equivalent" indexer using a discrete int and bool. (For reference, the int indexer is around 35ms).

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Nov 26, 2018

FWIW I apparently can't extend the span indexer bench to include these new Index cases without sorting the benchmark dependence issue (#16126). Or something like that.

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Nov 26, 2018

OSX failure is likely unrelated. Will see if it repros....

@dotnet-bot retest OSX10.12 x64 Checked Innerloop Build and Test

@tarekgh
Copy link
Member

tarekgh commented Nov 26, 2018

The Span changes looks good to me. I'll others review the jit changes as I am not familiar with.

briansull
briansull approved these changes Nov 26, 2018
View reviewed changes
Copy link

@briansull briansull left a comment

Choose a reason for hiding this comment

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

Looks Good

src/jit/assertionprop.cpp Outdated
return optAssertionProp_Update(newTree, tree, stmt);
}

// Else if we have an equality check involving a local, look for
Copy link

@briansull briansull Nov 26, 2018

Choose a reason for hiding this comment

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

At this point the comment should read:
Else check if we have an equality check involving a local

Or you could move this comment down to line 3123

@AndyAyersMS AndyAyersMS merged commit b01b2b8 into dotnet:master Nov 27, 2018
stephentoub pushed a commit to dotnet/corefx that referenced this pull request Nov 30, 2018
@AndyAyersMS @stephentoub
Improve perf for Index based span indexers (dotnet/coreclr#21196)
e0e8ca5 
First, evaluate the actual index before invoking the underlying int indexer,
so that the jit doesn't think the span is address taken.

Second, improve the jit's ability to remove some redundant comparisons like
those that arise in computing the actual index.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Anipik pushed a commit to dotnet/corert that referenced this pull request Nov 30, 2018
@AndyAyersMS @Anipik
Improve perf for Index based span indexers (dotnet/coreclr#21196)
51d4654 
First, evaluate the actual index before invoking the underlying int indexer,
so that the jit doesn't think the span is address taken.

Second, improve the jit's ability to remove some redundant comparisons like
those that arise in computing the actual index.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jlennox pushed a commit to jlennox/corefx that referenced this pull request Dec 16, 2018
@AndyAyersMS @jlennox
Improve perf for Index based span indexers (dotnet/coreclr#21196)
42ff7df 
First, evaluate the actual index before invoking the underlying int indexer,
so that the jit doesn't think the span is address taken.

Second, improve the jit's ability to remove some redundant comparisons like
those that arise in computing the actual index.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@AndyAyersMS AndyAyersMS mentioned this pull request Feb 11, 2019
Open
@AndyAyersMS AndyAyersMS mentioned this pull request Jan 31, 2020
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@AndyAyersMS
Improve perf for Index based span indexers (dotnet/coreclr#21196)
ca3e726 
First, evaluate the actual index before invoking the underlying int indexer,
so that the jit doesn't think the span is address taken.

Second, improve the jit's ability to remove some redundant comparisons like
those that arise in computing the actual index.


Commit migrated from dotnet/coreclr@b01b2b8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@briansull briansull briansull approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AndyAyersMS @tarekgh @briansull