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

Enable tree shaking of platform-specific code in AOT mode. #31969

Open
1 of 5 tasks
mraleph opened this issue Jan 23, 2018 · 9 comments
Open
1 of 5 tasks

Enable tree shaking of platform-specific code in AOT mode. #31969

mraleph opened this issue Jan 23, 2018 · 9 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@mraleph
Copy link
Member

mraleph commented Jan 23, 2018

Based on the converstation with Flutter users it seems that Flutter code it is common to guard large chunks of code behind runtime checks that boil down to:

if (Platform.isAndroid) {
  // ...
}

(actual flutter code might be using a layer on top: see https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/foundation/platform.dart)

Here's a list of subtasks:

/cc @lukef @mit-mit

@mraleph mraleph added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Jan 23, 2018
@mraleph
Copy link
Member Author

mraleph commented Jan 21, 2019

/cc @sstrickl - also see other issues linked from the code.

@mkustermann
Copy link
Member

Assigning @sstrickl . Please work with @lrhn and @mit-mit on finding the right solution here.

@mkustermann
Copy link
Member

The outcome of our offline discussion on Sept 29 was that we'll define a set of compiler-recognizable patterns for code that is dependent on platform/os/arch/... and then make our AOT compiler recognize those patterns to perform tree shaking on the kernel level.

@lrhn Have you thought about the set of patterns we should make our compiler support?

@lrhn
Copy link
Member

lrhn commented Oct 26, 2022

My idea is fairly simple: https://gist.github.com/lrhn/2a3b06a8253fddaf61297cb604c6777f

Basically, recognize isLinux and operatingSystem == "linux", and combinations of those like isLinux || isWindows.

@mkustermann
Copy link
Member

My idea is fairly simple: https://gist.github.com/lrhn/2a3b06a8253fddaf61297cb604c6777f

Basically, recognize isLinux and operatingSystem == "linux", and combinations of those like isLinux || isWindows.,

Thanks for writing it up, @lrhn !

What would be the code patterns we support on the usage site of those? (e.g. if (isLinux), caching isLinux in a local variable, using isLinux in other getters, more complicated control flow, ...)

Flutter is having it's own platform detection mechanism atm (it is exposing a defaultTargetPlatform getter that is based on _platform_io.dart which is currently even overridable at runtime by setting OS environment variable FLUTTER_TEST if assertions are enabled).

platform.TargetPlatform get defaultTargetPlatform {
  platform.TargetPlatform? result;
  if (Platform.isAndroid) {
    result = platform.TargetPlatform.android;
  } else if (Platform.isIOS) {
    ...
  }
  assert(() {
    if (Platform.environment.containsKey('FLUTTER_TEST')) {
      result = platform.TargetPlatform.android;
    }
    return true;
  }());
  if (platform.debugDefaultTargetPlatformOverride != null) {
    result = platform.debugDefaultTargetPlatformOverride;
  }
  if (result == null) throw ...;
  return result!;
}

Would we want to make our compiler recognize the above code to return a constant? Would we want to rewrite this implementation in flutter?

This flutter defaultTargetPlatform is used in practice to guard code, for example the flutter firebase plugin uses it here

@lrhn
Copy link
Member

lrhn commented Oct 27, 2022

I'd prefer to not make things too complicated, so only direct occurrences of isLinux inside the condition expression itself.
Remembering inferences from a boolean stored in a variable is possible (we've proven that for promotion), but it's also much more complicated and a little harder to read. The isX variables are short and directly useful.

I definitely don't want if (myPlatformTest()) { ... } to work, where myPlatformTest() => isLinux || isAndroid;. (If we ever make promoting methods work for type promotions, I'm willing to look at it again.)

As for more complicated control flow ... only if reasonable. (That would be recognizing escaping control flow.)
It's safe to not start with it. The warnings apply whenever you use a platform specific method, unless we can deduce that you can only be on a supported platform (can't rule out that you are on an unsupported platform). Making our analysis better in the future will not introduce more errors, only fewer.

I'd probably make the compiler just check for the operatingSystem == "linux" (and "linux" == operatingSystem), and force-inline any getter starting with is from the package:platform/platform.dart library. Then we can add more isGroupOfPlatforms => isFoo || isBar || isBaz; getters if necessary without needing to add extra support.

We can choose to accept assert(isLinux); too, for warnings, even if we can't use it for optimizations when asserts are not enabled.

Flutter could rewrite their target platform code to use our getters. It won't make the result constant, but it might be known at compile-time anyway, if the compiler knows the value of, say, isAndroid. That's a valid optimization.
Since they support overriding the actual runtime platform, it's different from what we do. The Dart compilers can safely optimize away non-platform code based on the values of the isX getters when it knows the value at runtime. The Flutter code above uses a non-const check for Platform.environment, so we can't do compile-time optimization based on the result of defaultTargetPlatform.

The goal here is two things:

  • Primary: Get warnings if using platform-specific code when not definitely on that platform.
    • This is a static check.
    • This is best effort.
    • We can decide on any number of ways to decide the current platform. Better analysis only removes warnings.
    • We can even use assert(isLinux) as a sign that the following code is definitely running on Linux, even if it's not safe.
  • Secondary: Optimize AoT tree-shaking by removing any code guarded by check for a platform other than the one being compiled for.
    • This too must be best effort.
    • It must be dynamically safe, so we can't rely on assert(isLinux) for truth in production, which is where you really want the tree-shaking.
    • It must not change program behavior, so we can't allow you to mock the current platform dynamically, it must at least go through the compiler.

If used correctly, all code that doesn't run on the current platform should be tree-shakable, because any use of it is definitely inside other code which can be tree-shakable.

@mkustermann
Copy link
Member

Flutter could rewrite their target platform code to use our getters. It won't make the result constant, ...

We have to be able to tree shake code that is guarded by platform checks. If flutter provides a blessed API that they recommend to users, users will use it, so we have to be able to tree shake based on that API.

(Seemingly this was originally added in flutter/flutter#33663)

@mkustermann
Copy link
Member

mkustermann commented Nov 15, 2022

Filed some sub-issues (see #31969 (comment))

@mkustermann mkustermann changed the title Optimize out Platform.isAndroid in AOT builds Enable tree shaking of platform-specific code in AOT mode. Nov 15, 2022
copybara-service bot pushed a commit that referenced this issue Feb 10, 2023
* Add an `targetOS` argument to `pkg/vm`'s `compileToKernel`,
  that contains the target operating system's name.

* Add a new `--target-os` command line argument for all binaries
  that use `compileToKernel` for clients to provide the target
  operating system, if known.

* Add a new`"vm:platform:const"` annotation to certain field and
  getters in the Platform class.

  This annotation is used to annotate static getters and fields with
  initializers where the getter body or field initializer must evaluate
  to a constant value if the target operating system is known. This
  annotation may be used outside the Platform class and in user code.

  For example, this annotation can be used on a static `String` field
  that is initialized with one value if `Platform.isWindows` is true
  and to a different value if `Platform.isWindows` is false.

  Note: If the const functions experimental flag is disabled, then
  any annotated static methods can only contain a single expression
  whose value is returned. If it is enabled, then the static method
  is evaluated as if it is a const function with the special
  handling of annotated static fields and getters above.

* Create a VM constant evaluator that evaluates uses of static getters
  and fields marked with the above annotations when a target operating
  system is provided.

* Use the new VM constant evaluator in the unreachable code elimination
  transformer.

TEST=pkg/vm/test/transformations/platform_use_transformer
     pkg/vm/test/transformations/unreachable_code_elimination

Change-Id: Ie381de70486a767fd7b1d515fd9e6bb58c6bf090
Bug: #31969
Cq-Include-Trybots: luci.dart.try:pkg-linux-release-try
CoreLibraryReviewExempt: Just adding vm-specific annotations.
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/274386
Commit-Queue: Tess Strickland <sstrickl@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
copybara-service bot pushed a commit that referenced this issue Nov 7, 2023
Namely, if the tested expression for a switch statement is constant,
then we can remove any constant cases where the constants differ,
and if all but a single case is removed, we can replace the switch
with the case body.

If constant functions are not enabled, then getters annotated with
@pragma("vm:platform-const") are still evaluated with the constant
function evaluation machinery, but only those and no others (including
any functions called within an annotated getter). This way, functions
can be annotated with @pragma("vm:platform-const") without having to
rewrite them to be a single returned expression.

TEST=pkg/vm/test/transformations/unreachable_code_elimination
     pkg/vm/test/transformations/vm_constant_evaluator

Issue: #50473
Issue: #31969
Change-Id: Ie290d2f1f469326238d66c3d9631f8e696685ff0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332760
Commit-Queue: Tess Strickland <sstrickl@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
copybara-service bot pushed a commit that referenced this issue Nov 7, 2023
…tor."

This reverts commit 92bf76d.

Reason for revert: broken tests on AOT trybots

Original change's description:
> [pkg/vm] Handle switch statements in unreachable code eliminator.
>
> Namely, if the tested expression for a switch statement is constant,
> then we can remove any constant cases where the constants differ,
> and if all but a single case is removed, we can replace the switch
> with the case body.
>
> If constant functions are not enabled, then getters annotated with
> @pragma("vm:platform-const") are still evaluated with the constant
> function evaluation machinery, but only those and no others (including
> any functions called within an annotated getter). This way, functions
> can be annotated with @pragma("vm:platform-const") without having to
> rewrite them to be a single returned expression.
>
> TEST=pkg/vm/test/transformations/unreachable_code_elimination
>      pkg/vm/test/transformations/vm_constant_evaluator
>
> Issue: #50473
> Issue: #31969
> Change-Id: Ie290d2f1f469326238d66c3d9631f8e696685ff0
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332760
> Commit-Queue: Tess Strickland <sstrickl@google.com>
> Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
> Reviewed-by: Alexander Markov <alexmarkov@google.com>

Issue: #50473
Issue: #31969
Change-Id: I4340874793da19ce892a19bcf0542c24e0ee65d4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/334600
Commit-Queue: Siva Annamalai <asiva@google.com>
Auto-Submit: Tess Strickland <sstrickl@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
copybara-service bot pushed a commit that referenced this issue Nov 9, 2023
…tor."

This is a reland of commit 92bf76d

In the original CL, the changes to the UCE assumed all
SwitchStatements were exhaustive. Now if a SwitchStatement isn't
explicitly or implicitly (via a default case) exhaustive and no case
matches the tested constant, the SwitchStatement is properly removed.

In addition, if a guaranteed to match case is found, more than one
cases remain (thus the SwitchStatement is not removed or replaced with
the single case's body), and the default case is removed, then the
resulting SwitchStatement is marked as explicitly exhaustive, as this
serves as a signal to backends that they do not need to handle the
possibility of no case matching in the absence of a default case.

Original change's description:
> [pkg/vm] Handle switch statements in unreachable code eliminator.
>
> Namely, if the tested expression for a switch statement is constant,
> then we can remove any constant cases where the constants differ,
> and if all but a single case is removed, we can replace the switch
> with the case body.
>
> If constant functions are not enabled, then getters annotated with
> @pragma("vm:platform-const") are still evaluated with the constant
> function evaluation machinery, but only those and no others (including
> any functions called within an annotated getter). This way, functions
> can be annotated with @pragma("vm:platform-const") without having to
> rewrite them to be a single returned expression.
>
> TEST=pkg/vm/test/transformations/unreachable_code_elimination
>      pkg/vm/test/transformations/vm_constant_evaluator
>
> Issue: #50473
> Issue: #31969
> Change-Id: Ie290d2f1f469326238d66c3d9631f8e696685ff0
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332760
> Commit-Queue: Tess Strickland <sstrickl@google.com>
> Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
> Reviewed-by: Alexander Markov <alexmarkov@google.com>

TEST=pkg/vm/test/transformations/unreachable_code_elimination
     pkg/vm/test/transformations/vm_constant_evaluator

Issue: #50473
Issue: #31969
Cq-Include-Trybots: luci.dart.try:vm-aot-linux-release-x64-try,vm-aot-mac-release-arm64-try
Change-Id: I557ca933808012e670e306f2d880221a0d7dd670
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/334224
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Tess Strickland <sstrickl@google.com>
copybara-service bot pushed a commit that referenced this issue Nov 13, 2023
… eliminator.""

This reverts commit 2623117.

Reason for revert: Breaks legacy mode.
Bug: #54029

Original change's description:
> Reland "[pkg/vm] Handle switch statements in unreachable code eliminator."
>
> This is a reland of commit 92bf76d
>
> In the original CL, the changes to the UCE assumed all
> SwitchStatements were exhaustive. Now if a SwitchStatement isn't
> explicitly or implicitly (via a default case) exhaustive and no case
> matches the tested constant, the SwitchStatement is properly removed.
>
> In addition, if a guaranteed to match case is found, more than one
> cases remain (thus the SwitchStatement is not removed or replaced with
> the single case's body), and the default case is removed, then the
> resulting SwitchStatement is marked as explicitly exhaustive, as this
> serves as a signal to backends that they do not need to handle the
> possibility of no case matching in the absence of a default case.
>
> Original change's description:
> > [pkg/vm] Handle switch statements in unreachable code eliminator.
> >
> > Namely, if the tested expression for a switch statement is constant,
> > then we can remove any constant cases where the constants differ,
> > and if all but a single case is removed, we can replace the switch
> > with the case body.
> >
> > If constant functions are not enabled, then getters annotated with
> > @pragma("vm:platform-const") are still evaluated with the constant
> > function evaluation machinery, but only those and no others (including
> > any functions called within an annotated getter). This way, functions
> > can be annotated with @pragma("vm:platform-const") without having to
> > rewrite them to be a single returned expression.
> >
> > TEST=pkg/vm/test/transformations/unreachable_code_elimination
> >      pkg/vm/test/transformations/vm_constant_evaluator
> >
> > Issue: #50473
> > Issue: #31969
> > Change-Id: Ie290d2f1f469326238d66c3d9631f8e696685ff0
> > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332760
> > Commit-Queue: Tess Strickland <sstrickl@google.com>
> > Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
> > Reviewed-by: Alexander Markov <alexmarkov@google.com>
>
> TEST=pkg/vm/test/transformations/unreachable_code_elimination
>      pkg/vm/test/transformations/vm_constant_evaluator
>
> Issue: #50473
> Issue: #31969
> Cq-Include-Trybots: luci.dart.try:vm-aot-linux-release-x64-try,vm-aot-mac-release-arm64-try
> Change-Id: I557ca933808012e670e306f2d880221a0d7dd670
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/334224
> Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
> Reviewed-by: Alexander Markov <alexmarkov@google.com>
> Commit-Queue: Tess Strickland <sstrickl@google.com>

Issue: #50473
Issue: #31969
Change-Id: I56373c7a6feac76e23c1800ae83eb013c5856cba
Cq-Include-Trybots: luci.dart.try:vm-aot-linux-release-x64-try,vm-aot-mac-release-arm64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/335820
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
copybara-service bot pushed a commit that referenced this issue Nov 21, 2023
…tor."

This is a reland of commit 2623117

When running with sound null safety, reducing a logical expression to
the right hand side if the left hand side is constant and does not short
circuit is valid, as the right hand side is guaranteed not to evaluate
to null.

In other modes, however, the right hand side may evaluate to null.
Thus, in thos modes, we return the original node if the RHS is not a
constant boolean value, so that the operator can perform whatever null
checking is required.

Fixes: #54029

Original change's description:
> Reland "[pkg/vm] Handle switch statements in unreachable code eliminator."
>
> This is a reland of commit 92bf76d
>
> In the original CL, the changes to the UCE assumed all
> SwitchStatements were exhaustive. Now if a SwitchStatement isn't
> explicitly or implicitly (via a default case) exhaustive and no case
> matches the tested constant, the SwitchStatement is properly removed.
>
> In addition, if a guaranteed to match case is found, more than one
> cases remain (thus the SwitchStatement is not removed or replaced with
> the single case's body), and the default case is removed, then the
> resulting SwitchStatement is marked as explicitly exhaustive, as this
> serves as a signal to backends that they do not need to handle the
> possibility of no case matching in the absence of a default case.
>
> Original change's description:
> > [pkg/vm] Handle switch statements in unreachable code eliminator.
> >
> > Namely, if the tested expression for a switch statement is constant,
> > then we can remove any constant cases where the constants differ,
> > and if all but a single case is removed, we can replace the switch
> > with the case body.
> >
> > If constant functions are not enabled, then getters annotated with
> > @pragma("vm:platform-const") are still evaluated with the constant
> > function evaluation machinery, but only those and no others (including
> > any functions called within an annotated getter). This way, functions
> > can be annotated with @pragma("vm:platform-const") without having to
> > rewrite them to be a single returned expression.
> >
> > TEST=pkg/vm/test/transformations/unreachable_code_elimination
> >      pkg/vm/test/transformations/vm_constant_evaluator
> >
> > Issue: #50473
> > Issue: #31969
> > Change-Id: Ie290d2f1f469326238d66c3d9631f8e696685ff0
> > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332760
> > Commit-Queue: Tess Strickland <sstrickl@google.com>
> > Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
> > Reviewed-by: Alexander Markov <alexmarkov@google.com>
>
> TEST=pkg/vm/test/transformations/unreachable_code_elimination
>      pkg/vm/test/transformations/vm_constant_evaluator
>
> Issue: #50473
> Issue: #31969
> Cq-Include-Trybots: luci.dart.try:vm-aot-linux-release-x64-try,vm-aot-mac-release-arm64-try
> Change-Id: I557ca933808012e670e306f2d880221a0d7dd670
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/334224
> Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
> Reviewed-by: Alexander Markov <alexmarkov@google.com>
> Commit-Queue: Tess Strickland <sstrickl@google.com>

TEST=pkg/vm/test/transformations/unreachable_code_elimination
     pkg/vm/test/transformations/vm_constant_evaluator

Change-Id: Ia51b7c5f3b51f57a6a306551fe74b47e0cba3c23
Cq-Include-Trybots: luci.dart.try:vm-aot-linux-release-x64-try,vm-aot-mac-release-arm64-try,vm-kernel-precomp-linux-release-x64-try,vm-kernel-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/335828
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Tess Strickland <sstrickl@google.com>
copybara-service bot pushed a commit that referenced this issue Jan 30, 2024
This allows more complicated field initializers that are written using
immediately invoked closures, like

@pragma("vm:platform-const")
final bool foo = () {
  ... do some stuff ...
}();

to be properly evaluated by the VM constant evaluator.

Also throws errors at compile time if the annotated member cannot be
evaluated to a constant or if an invalid member (not an
initialized static field or a static getter) is annotated.

TEST=pkg/vm/test/transformations/vm_constant_evaluator_test

Issue: #31969
Issue: #50473
Issue: flutter/flutter#14233
Change-Id: I14be498bb5f7771f0f339baf7d3b1bec7df5903f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/348380
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Tess Strickland <sstrickl@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

4 participants