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

Consider disabling Ref.Emit in System.Linq.Expressions on WASM #38439

Open
MichalStrehovsky opened this issue Jun 26, 2020 · 18 comments
Open

Consider disabling Ref.Emit in System.Linq.Expressions on WASM #38439

MichalStrehovsky opened this issue Jun 26, 2020 · 18 comments
Labels
arch-wasm WebAssembly architecture area-System.Linq.Expressions size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@MichalStrehovsky
Copy link
Member

I was surprised the default Blazor WASM template depends on Reflection.Emit. This is because of System.Linq.Expressions.

System.Linq.Expressions can be compiled in a way that removes the Ref.Emit dependency (use interpreter only) - to do this set the IsInterpreting property in System.Linq.Expressions.csproj to true.

Compiling without Ref.Emit support saves 112 kB uncompressed and 44 kB compressed on System.Linq.Expressions.dll alone. I expect there will be more savings possible in CoreLib (the Ref.Emit implementation).

#38438 fixes some bitrot in System.Linq.Expressions that is necessary to build this way.

Cc @marek-safar @eerhardt

@MichalStrehovsky MichalStrehovsky added this to the 5.0.0 milestone Jun 26, 2020
@ghost
Copy link

ghost commented Jun 26, 2020

Tagging subscribers to this area: @cston
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jun 26, 2020
@marek-safar
Copy link
Contributor

I was surprised the default Blazor WASM template depends on Reflection.Emit. This is because of System.Linq.Expressions.

I'm sad every time I see System.Linq.Expressions in the default template of size sensitive workloads. It might be worth checking how to make that conditional in Blazor.

System.Linq.Expressions can be compiled in a way that removes the Ref.Emit dependency (use interpreter only) - to do this set the IsInterpreting property in System.Linq.Expressions.csproj to true.

I think it'd be better to have this instead of compile-time constant use feature-switch RuntimeFeature.IsDynamicCodeCompiled as this is not arch but codegen switch we could use elsewhere.

Compiling without Ref.Emit support saves 112 kB uncompressed and 44 kB compressed on System.Linq.Expressions.dll alone.

We actually switched from interpreter to SRE a few years ago for Mono profile due to performance improvements but it's possible that for NET Core libraries the situation will be different. However, we need to test if the behaviour is the same and where we stand on performance.

@MichalStrehovsky
Copy link
Member Author

I'm sad every time I see System.Linq.Expressions in the default template of size sensitive workloads. It might be worth checking how to make that conditional in Blazor.

Seems to be coming from Microsoft.Extensions.DependencyInjection 😞

@benaadams
Copy link
Member

I'm sad every time I see System.Linq.Expressions in the default template of size sensitive workloads. It might be worth checking how to make that conditional in Blazor.

Seems to be coming from Microsoft.Extensions.DependencyInjection 😞

Might need correct flags setting for WASM?

<PropertyGroup>
<ILEmitBackend Condition="'$(TargetFramework)' != 'netstandard2.0'">True</ILEmitBackend>
<DefineConstants Condition="'$(ILEmitBackend)' == 'True'">$(DefineConstants);IL_EMIT</DefineConstants>
<DefineConstants Condition="$(TargetFramework.StartsWith('net4')) and '$(ILEmitBackendSaveAssemblies)' == 'True'">$(DefineConstants);SAVE_ASSEMBLIES</DefineConstants>
</PropertyGroup>

@MichalStrehovsky
Copy link
Member Author

Might need correct flags setting for WASM?

Ah, I meant - the reference to Linq.Expressions is coming from DependencyInjection.

The fact that DependencyInjection can also use Ref.Emit is new information to me, but makes perfect sense (if you want bad startup time and DI by itself is still not slow enough, might want to insure it's really bad by adding Ref.Emit to the mix).

@benaadams
Copy link
Member

The fact that DependencyInjection can also use Ref.Emit is new information to me, but makes perfect sense (if you want bad startup time and DI by itself is still not slow enough, might want to insure it's really bad by adding Ref.Emit to the mix).

😂 I think @migueldeicaza would sympathize with this position

@eerhardt eerhardt added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 1, 2020
@eerhardt eerhardt self-assigned this Jul 1, 2020
@eerhardt eerhardt removed their assignment Jul 1, 2020
@eerhardt
Copy link
Member

eerhardt commented Jul 1, 2020

Seems to be coming from Microsoft.Extensions.DependencyInjection 😞

I've logged #38678 to make DependencyInjection more trimmable. Fixing that will remove Linq.Expressions in a default Blazor WASM template app. After that, the only usage of Ref.Emit left is #38693.

@MichalStrehovsky
Copy link
Member Author

Per Steve Sanderson, Blazor WASM is going to bring in a dependency on expressions as soon as someone drops a form into their app: https://github.com/dotnet/aspnetcore/blob/master/src/Components/Web/src/Forms/InputBase.cs#L51

@marek-safar
Copy link
Contributor

I still think the best option is to have both options in and control it via RuntimeFeature.IsDynamicCodeCompiled. I believe we do it in other parts of libraries as well, for example for regular expressions.

@eerhardt eerhardt removed the untriaged New issue has not been triaged by the area owner label Jul 9, 2020
@stephentoub
Copy link
Member

stephentoub commented Jul 10, 2020

I still think the best option is to have both options in and control it via RuntimeFeature.IsDynamicCodeCompiled. I believe we do it in other parts of libraries as well, for example for regular expressions.

I'm really curious about RuntimeFeature.IsDynamicCodeCompiled here. I added its use into regex based on such a recommendation, but it actually seems like the wrong choice when running with an interpreter.

Often reflection emit (or S.L.Expressions) is used when there's some general implementation and then a very specific (and thus faster / more tuned) implementation can be emitted for a specific use case. That's what happens with regex. Now, if the choice is between using a compiled version of the general path and an interpreted version of the specific path, it's really hard to say which is going to be better. But if the choice is between using an interpreted version of the general path and an interpreted version of the specific path, in general I'd expect the latter to win. After all, with something like regex, is it better for the interpreter to execute the interpreter's IL interpreting the regex instructions, or is it better for the interpreter to execute the IL specific to that regex. There's still the overhead of doing the IL generation, but that's the case whether the IL generation path is interpreted or compiled.

Do we need a RuntimeFeature.IsInterpreting, or some such thing?

@eerhardt
Copy link
Member

But if the choice is between using an interpreted version of the general path and an interpreted version of the specific path, in general I'd expect the latter to win

This was the same case for Json serialization. See my perf results when I tested using Ref.Emit vs. Reflection to get/set properties and create objects. The Ref.Emit case (after warm-up) was 15-20% faster.

Do we need a RuntimeFeature.IsInterpreting, or some such thing?

What decisions would we make off that property that we wouldn't make with RuntimeFeature.IsDynamicCodeCompiled?

@jkotas
Copy link
Member

jkotas commented Jul 10, 2020

It is even more complex once you throw the fact that some code may be compiled (JIT or AOT) and some code may be interpreted. We are not there today, but it is where we want to be with wasm eventually.

There is typically transition cost between interpreting and running compiled code. So whether the regex compiled into IL running on interpreter is going to be faster than the built-in regex interpretation is also going to depend on how chatty is the interaction between the regex IL and static libraries that are likely to be compiled.

The interpreter is always going to be a low-throughput configuration. I think we should make it simple and prioritize size over speed for the interpreter. The high-throughput option for Wasm would be full AOT.

@stephentoub
Copy link
Member

What decisions would we make off that property that we wouldn't make with RuntimeFeature.IsDynamicCodeCompiled?

Let's take regex as an example.

  1. If the whole app is interpreted, then I'd bet ref emit is actually beneficial for throughput (after warm-up), since it'll be interpreting the faster rather than the slower path.
  2. If the whole app is compiled, but dynamic code isn't compiled but instead interpreted, then it's not as clear cut: is it faster to run the compiled regex interpreter or faster to run the interpreted ref emited impl?

@marek-safar
Copy link
Contributor

If the whole app is interpreted, then I'd bet ref emit is actually beneficial for throughput

That's not how you should look at it with the tech we have today. The interpreter is quite limited in how much optimizations/propagations can do. So whether the code is ref-emit generated or precompiled does not matter, what matters is how much code needs to process. In the simple view, you can say that every IL instruction cost you 1 us to execute, if you can write a program in the way that it will be 100 IL instructions without ref-emit and 150 with ref-emit (ignoring the ref-emit generation part) the winner will be no-ref-emit version.

@stephentoub
Copy link
Member

Yes, and for Regex for example, there are in general many fewer IL instructions spit out for the ref emit version than need to be executed for the interpreted version. There are fewer IL instructions to execute in the ref emited IL. That's exactly my point.

@eerhardt
Copy link
Member

In order for Ref.Emit to be completely trimmed in a Blazor application, we would also need to implement #38693. #38693 has perf tradeoffs that aren't desirable at this time. So moving both this and #38693 to 6.0.

@eerhardt eerhardt modified the milestones: 5.0.0, 6.0.0 Jul 30, 2020
@marek-safar marek-safar added size-reduction Issues impacting final app size primary for size sensitive workloads and removed linkable-framework Issues associated with delivering a linker friendly framework labels Dec 9, 2020
@eerhardt
Copy link
Member

Moving to 'Future' as this isn't high priority for 6.

@eerhardt eerhardt modified the milestones: 6.0.0, Future May 11, 2021
@Thaina
Copy link

Thaina commented Jun 24, 2021

If the Reflection.Emit got trimmed, how can we enable it again when we want to have wasm emit dynamic code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly architecture area-System.Linq.Expressions size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

No branches or pull requests

8 participants