Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decorate SequenceEqual with AggressiveOptimization #79645

Closed
wants to merge 1 commit into from

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Dec 14, 2022

I'm not sure this is good idea or not, I'm looking for feedback.

We found out with @ilonatommy that string.Normalize could be slow when running on Mono interp (in browser).
The reason is that interp would tier up and optimize the SequenceEqual method only after 1000 calls.
If we add [MethodImpl(MethodImplOptions.AggressiveOptimization)] the Mono interpreter would optimize it right away.

Many of the other methods in the SpanHelpers are decorated same way. Is SequenceEqual just missing the attribute ?

At this point I don't know what is customer impact.

@ghost
Copy link

ghost commented Dec 14, 2022

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

Issue Details

I'm not sure this is good idea or not, I'm looking for feedback.

We found out with @ilonatommy that string.Normalize could be slow when running on Mono interp (in browser).
The reason is that interp would tier up and optimize the method only after 1000 calls.
If we add [MethodImpl(MethodImplOptions.AggressiveOptimization)] the Mono interpreter would optimize it right away.

Many of the other methods in the SpanHelpers are decorated same way. Is SequenceEqual just missing the attribute ?

At this point I don't know what is customer impact.

Author: pavelsavara
Assignees: pavelsavara
Labels:

area-System.Memory, optimization

Milestone: 8.0.0

@EgorBo
Copy link
Member

EgorBo commented Dec 14, 2022

I don't think we need any of the [AggressiveOptimization] in SpanHelpers for CoreCLR runtime. Perhaps, only those with Vector<> acceleration only as those won't be prejitted. R2R'd version of SpanHelpers is expected to be fast (even if it doesn't use Vector256 path) so we don't need to waste time JITtting them on start.

So far, for CoreCLR we only have two justifications for [AggressiveOptimization]:

  1. Tier0 or R2R'd version allocates while Tier1 does not
  2. Some CastHelpers have to be [AO] because VM/JIT are able to emit them as direct calls.

@jkotas
Copy link
Member

jkotas commented Dec 14, 2022

AggressiveOptimization has a problematic behavior for CoreCLR. It disables AOT compilation (makes the startup worse) and tiered compilation (makes the steady state throughput worse). The number of places where it can be used with clear benefits is very narrow.

I don't think we need any of the [AggressiveOptimization] in SpanHelpers for CoreCLR runtime.

We have #71261 opened on this.

@radical
Copy link
Member

radical commented Dec 14, 2022

Are there existing benchmarks where the improvement can be seen, or a future regression caught?

@pavelsavara
Copy link
Member Author

Are there existing benchmarks where the improvement can be seen, or a future regression caught?

I don't know, we just explored one-off scenario locally.

Thanks everybody for the explanation and the context. I'm closing it as I don't have strong evidence this has visible impact on wasm and it would probably have impact on CLR.

@EgorBo
Copy link
Member

EgorBo commented Dec 14, 2022

Are there existing benchmarks where the improvement can be seen, or a future regression caught?

I don't know, we just explored one-off scenario locally.

Thanks everybody for the explanation and the context. I'm closing it as I don't have strong evidence this has visible impact on wasm and it would probably have impact on CLR.

Well, it raised a good question whether we should preserve existing [AO] for Mono if we decide to drop them for CoreCLR or not 🙂

@ghost ghost locked as resolved and limited conversation to collaborators Jan 13, 2023
@pavelsavara pavelsavara deleted the optimize_SequenceEqual branch September 2, 2024 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants