Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Allow creating slimpeller engine variants. #51824

Merged
merged 2 commits into from
Apr 2, 2024
Merged

Allow creating slimpeller engine variants. #51824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e509e34
Allow creating slimpeller engine variants.
chinmaygarde Apr 1, 2024
7d6b10a
Add links to the project.
chinmaygarde Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/config.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ declare_args() {
#TODO(cyanglaz): Remove above comment about test flag when the entire iOS embedder supports app extension
#https://github.com/flutter/flutter/issues/124289
darwin_extension_safe = false

# Whether binary size optimizations with the assumption that Impeller is the
# only supported rendering engine are enabled.
#
# See [go/slimpeller-dashboard](https://github.com/orgs/flutter/projects/21)
# for details.
slimpeller = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, let's use a less cute name, like remove_skia_gpu_backend please.

Copy link
Member

Choose a reason for hiding this comment

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

Guessing that this is going to be used for ifdef guards around the Skia GPU code. In that case, we might want to avoid double negatives (#ifndef REMOVE_SKIA_GPU) and instead call it include_skia_gpu so that it would be #ifdef INCLUDE_SKIA_GPU in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah seems reasonable, just something that isn't a codename or a double negative!

Copy link
Member Author

Choose a reason for hiding this comment

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

The dashboard tracks more than just removing Skia GPU with this effort (like removing the built-in codecs as well as the Skia software backend).

let's use a less cute name

Why? Did I miss a memo? To borrow your phrase, I am not taking suggestions for an alternate name at this time.

Copy link
Contributor

@matanlurey matanlurey Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you are making a joke and I don't get the reference (the link you provided is an internal link that probably should not be linked here in general) feel free to ignore.... but if not - this is team project, and I would not possibly be able to know that "slimpeller = true" means, for example, the built-in codecs change, and I suspect that goes the same for our gardeners and rollers.

If include_skia_gpu_and_built_in_codecs is not sufficient, then another idea is to break it into two flags (include_skia_gpu, include_skia_built_in_codecs).

tl;dr: Not trying to be persnickety, I'm trying to make future debugging easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

The link I provided is obfuscated behind a UUID. So you can't read it if you aren't on the channel (you are). But, yeah, the context is that that we added DeathRattle as an alternative to fml::ScopedCleanupClosure. Thought it was cute (and less readable, made future debugging harder as you put it). Which is to say, I don't think there is a moratorium on using cute names in the codebase. So I reject the notion that we should stop introducing them now.

Back to the point though, I need one simple engine variant I can wire up on CI where the various action items listed on the dashboard can be implemented and their size binary size impact tracked. All items related to this effort are tracked on a dashboard and labelled. What the flag does is documented in GN file as well as the help string for ./flutter/tools/gn. Trying to figure out what slimpeller is supposed to be is trivial. include_skia_gpu_and_built_in_codecs is pedantic and as the effort continues to evolve and, say implements the removal of the codecs, becomes out of date. Hence the name.

I am not trying to do this to be cute. Need one variant I can put on CI that tracks an evolving effort (that does more than Skia GPU removal).

Copy link
Contributor

@matanlurey matanlurey Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

aside: I am perfectly fine with renaming DeathRattle (which is a test only appliance that cannot impact user code) - flutter/flutter#146105.

I strongly don't think we should be using code names like this. An engine variant does not have to be tied to a single flag (you can setup the variant and flip the flags however you want), that is, you can create a slimpeller variant that turns both the flags to false.

What I am asking for is to pick flags that accurately explain what something is doing, in so far that a gardener, sheriff, google3 roller, or even a client name downstream will be able to understand why something might have impacted or broke them. This is a very reasonable request, but I'm also happy to bring up at the weekly and get other opinions if comes to that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Impeller is a codename, so is flow, so is Skia. I obviously feel less strongly about codenames. No Flutter user/developer cares about these. But these are already part of the lexicon of the sheriffs and gardeners (who are neither real sheriffs nor gardners).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, request withdrawn.

}

# feature_defines_list ---------------------------------------------------------
Expand Down Expand Up @@ -62,6 +69,10 @@ if (flutter_runtime_mode == "debug") {
feature_defines_list += [ "FLUTTER_RUNTIME_MODE=0" ]
}

if (slimpeller) {
feature_defines_list += [ "SLIMPELLER=1" ]
}

if (is_ios || is_mac) {
flutter_cflags_objc = [
"-Werror=overriding-method-mismatch",
Expand Down
12 changes: 12 additions & 0 deletions tools/gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ def get_out_dir(args):
if args.darwin_extension_safe:
target_dir.append('extension_safe')

if args.slimpeller:
target_dir.append('slimpeller')

if args.target_dir != '':
target_dir = [args.target_dir]

Expand Down Expand Up @@ -1260,6 +1263,15 @@ def parse_args(args):
help='Do not run GN. Instead configure the Impeller cmake example build.',
)

parser.add_argument(
'--slimpeller',
default=False,
action='store_true',
help='Enable optimizations that attempt to reduce binary size of the ' +
'Flutter engine by assuming only the Impeller rendering engine is supported.' +
'See [go/slimpeller-dashboard](https://github.com/orgs/flutter/projects/21) for details.'
)

# Sanitizers.
parser.add_argument('--asan', default=False, action='store_true')
parser.add_argument('--lsan', default=False, action='store_true')
Expand Down