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

Remove FEATURE_COMPILE define constant from System.Linq.Expressions #32878

Closed
cston opened this issue Feb 26, 2020 · 10 comments
Closed

Remove FEATURE_COMPILE define constant from System.Linq.Expressions #32878

cston opened this issue Feb 26, 2020 · 10 comments

Comments

@cston
Copy link
Member

cston commented Feb 26, 2020

FEATURE_COMPILE is defined on all platforms so the define constant can be removed along with the #else blocks from any #if FEATURE_COMPILE.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@cston cston removed the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@cston cston added this to the 5.0 milestone Feb 26, 2020
@jkotas
Copy link
Member

jkotas commented Feb 26, 2020

@marek-safar Is this going to be needed for some of the Mono flavors? (Blazor, Xamarin iOS, ...)

@marek-safar
Copy link
Contributor

We don't plan to use the SLE interpreter in .net5. The runtime interpreter should work fine with the "compile" version.

@stephentoub
Copy link
Member

What about for size? Would a build without the compiler be significantly smaller enough to warrant building without it on some platforms? Or we think we could remove it via the linker based on how the Compile method is called?

@stephentoub
Copy link
Member

(I have a similar question about System.Text.RegularExpressions, which also has a very similar ifdef for its compiler.)

@marek-safar
Copy link
Contributor

My understanding, in this case, is that the non-compile version brings whole SLE interpreter which is a chunk of code as well. If we want to optimize for size, in this case, it should be with e.g. RuntimeFeature.IsDynamicCodeCompiled condition which ILLinker knowns how to remove.

@stephentoub
Copy link
Member

If we want to optimize for size, in this case, it should be with e.g. RuntimeFeature.IsDynamicCodeCompiled condition which ILLinker knowns how to remove.

And there aren't any platforms where IsDynamicCodeCompiled returns true but we still care about size?

If we can have a single build, removing the ifdefs, and then let the linker exclude stuff to optimize for size, that's great. I just want to make sure we use the right guard condition.

The recommendation then is, anywhere we or anyone else has both an interpreter and a compiler implementation for the same functionally, that they both be built in and the compiler guarded behind IsDynamicCodeCompiled?

@marek-safar
Copy link
Contributor

marek-safar commented Feb 27, 2020

And there aren't any platforms where IsDynamicCodeCompiled returns true but we still care about size?

We have only Android in that camp but the speed improvement from the compiled code there outweighs potential size increase.

@stephentoub
Copy link
Member

cc: @MichalStrehovsky

@cston
Copy link
Member Author

cston commented Aug 13, 2020

Moving to 6.0.

Please follow up if this needs to be addressed sooner.

@cston cston modified the milestones: 5.0.0, 6.0.0 Aug 13, 2020
@cston cston modified the milestones: 6.0.0, 7.0.0 Aug 10, 2021
@cston cston modified the milestones: 7.0.0, Future Aug 15, 2022
@jkotas
Copy link
Member

jkotas commented Dec 16, 2022

FEATURE_COMPILE define was deleted in #61952.

@jkotas jkotas closed this as completed Dec 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants