Unverified Commit c55b3220 authored by Jonah Williams's avatar Jonah Williams Committed by GitHub

[flutter_tools] fix performance of tree-shake-icons (#55417)

Fixes the performance issue with tree-shake-icons and filters to ttf mime type. Does not change default or error behavior.
parent 72397fd4
......@@ -60,9 +60,9 @@ Future<Depfile> copyAssets(Environment environment, Directory outputDirectory) a
file.parent.createSync(recursive: true);
final DevFSContent content = entry.value;
if (content is DevFSFileContent && content.file is File) {
inputs.add(globals.fs.file(content.file.path));
inputs.add(content.file as File);
if (!await iconTreeShaker.subsetFont(
inputPath: content.file.path,
input: content.file as File,
outputPath: file.path,
relativePath: entry.key,
)) {
......
......@@ -4,6 +4,7 @@
import 'package:meta/meta.dart';
import 'package:process/process.dart';
import 'package:mime/mime.dart' as mime;
import '../../artifacts.dart';
import '../../base/common.dart';
......@@ -66,6 +67,12 @@ class IconTreeShaker {
}
}
/// The MIME type for ttf fonts.
static const Set<String> kTtfMimeTypes = <String>{
'font/ttf', // based on internet search
'application/x-font-ttf', // based on running locally.
};
/// The [Source] inputs that targets using this should depend on.
///
/// See [Target.inputs].
......@@ -77,6 +84,7 @@ class IconTreeShaker {
final Environment _environment;
final String _fontManifest;
Future<void> _iconDataProcessing;
Map<String, _IconTreeShakerData> _iconData;
final ProcessManager _processManager;
......@@ -89,10 +97,10 @@ class IconTreeShaker {
&& _environment.defines[kIconTreeShakerFlag] == 'true'
&& _environment.defines[kBuildMode] != 'debug';
/// Fills the [_iconData] map.
Future<Map<String, _IconTreeShakerData>> _getIconData(Environment environment) async {
// Fills the [_iconData] map.
Future<void> _getIconData(Environment environment) async {
if (!enabled) {
return null;
return;
}
final File appDill = environment.buildDir.childFile('app.dill');
......@@ -119,12 +127,14 @@ class IconTreeShaker {
);
if (fonts.length != iconData.length) {
throwToolExit('Expected to find fonts for ${iconData.keys}, but found '
'${fonts.keys}. This usually means you are refering to '
'font families in an IconData class but not including them '
'in the assets section of your pubspec.yaml, are missing '
'the package that would include them, or are missing '
'"uses-material-design: true".');
throwToolExit(
'Expected to find fonts for ${iconData.keys}, but found '
'${fonts.keys}. This usually means you are refering to '
'font families in an IconData class but not including them '
'in the assets section of your pubspec.yaml, are missing '
'the package that would include them, or are missing '
'"uses-material-design: true".',
);
}
final Map<String, _IconTreeShakerData> result = <String, _IconTreeShakerData>{};
......@@ -135,13 +145,11 @@ class IconTreeShaker {
codePoints: iconData[entry.key],
);
}
return result;
_iconData = result;
}
/// Calls font-subset, which transforms the `inputPath` font file to a
/// subsetted version at `outputPath`.
///
/// The `relativePath` parameter
/// Calls font-subset, which transforms the [input] font file to a
/// subsetted version at [outputPath].
///
/// All parameters are required.
///
......@@ -150,15 +158,24 @@ class IconTreeShaker {
/// If the font-subset subprocess fails, it will [throwToolExit].
/// Otherwise, it will return true.
Future<bool> subsetFont({
@required String inputPath,
@required File input,
@required String outputPath,
@required String relativePath,
}) async {
if (!enabled) {
return false;
}
_iconData ??= await _getIconData(_environment);
if (input.lengthSync() < 12) {
return false;
}
final String mimeType = mime.lookupMimeType(
input.path,
headerBytes: await input.openRead(0, 12).first,
);
if (!kTtfMimeTypes.contains(mimeType)) {
return false;
}
await (_iconDataProcessing ??= _getIconData(_environment));
assert(_iconData != null);
final _IconTreeShakerData iconTreeShakerData = _iconData[relativePath];
......@@ -176,7 +193,7 @@ class IconTreeShaker {
final List<String> cmd = <String>[
fontSubset.path,
outputPath,
inputPath,
input.path,
];
final String codePoints = iconTreeShakerData.codePoints.join(' ');
_logger.printTrace('Running font-subset: ${cmd.join(' ')}, '
......@@ -186,9 +203,7 @@ class IconTreeShaker {
fontSubsetProcess.stdin.writeln(codePoints);
await fontSubsetProcess.stdin.flush();
await fontSubsetProcess.stdin.close();
} on Exception catch (_) {
// handled by checking the exit code.
} on OSError catch (_) { // ignore: dead_code_on_catch_subtype
} on Exception {
// handled by checking the exit code.
}
......
......@@ -136,7 +136,7 @@ class BuildBundleCommand extends BuildSubCommand {
extraGenSnapshotOptions: buildInfo.extraGenSnapshotOptions,
fileSystemScheme: stringArg('filesystem-scheme'),
fileSystemRoots: stringsArg('filesystem-root'),
treeShakeIcons: boolArg('tree-shake-icons'),
treeShakeIcons: buildInfo.treeShakeIcons,
);
return FlutterCommandResult.success();
}
......
......@@ -19,7 +19,7 @@ class BuildWebCommand extends BuildSubCommand {
BuildWebCommand({
@required bool verboseHelp,
}) {
addTreeShakeIconsFlag();
addTreeShakeIconsFlag(enabledByDefault: false);
usesTargetOption();
usesPubOption();
addBuildModeFlags(excludeDebug: true);
......
......@@ -425,10 +425,13 @@ abstract class FlutterCommand extends Command<void> {
);
}
void addTreeShakeIconsFlag() {
void addTreeShakeIconsFlag({
bool enabledByDefault
}) {
argParser.addFlag('tree-shake-icons',
negatable: true,
defaultsTo: kIconTreeShakerEnabledDefault,
defaultsTo: enabledByDefault
?? kIconTreeShakerEnabledDefault,
help: 'Tree shake icon fonts so that only glyphs used by the application remain.',
);
}
......@@ -579,8 +582,12 @@ abstract class FlutterCommand extends Command<void> {
'combination with "--${FlutterOptions.kSplitDebugInfoOption}"',
);
}
final BuildMode buildMode = getBuildMode();
final bool treeShakeIcons = argParser.options.containsKey('tree-shake-icons')
&& buildMode.isPrecompiled
&& boolArg('tree-shake-icons');
return BuildInfo(getBuildMode(),
return BuildInfo(buildMode,
argParser.options.containsKey('flavor')
? stringArg('flavor')
: null,
......@@ -601,9 +608,7 @@ abstract class FlutterCommand extends Command<void> {
buildName: argParser.options.containsKey('build-name')
? stringArg('build-name')
: null,
treeShakeIcons: argParser.options.containsKey('tree-shake-icons')
? boolArg('tree-shake-icons')
: kIconTreeShakerEnabledDefault,
treeShakeIcons: treeShakeIcons,
splitDebugInfoPath: splitDebugInfoPath,
dartObfuscation: dartObfuscation,
dartDefines: argParser.options.containsKey(FlutterOptions.kDartDefinesOption)
......
......@@ -23,38 +23,39 @@ import '../../../src/common.dart';
import '../../../src/context.dart';
import '../../../src/mocks.dart' as mocks;
final Platform _kNoAnsiPlatform = FakePlatform(stdoutSupportsAnsi: false);
final Platform kNoAnsiPlatform = FakePlatform(stdoutSupportsAnsi: false);
const List<int> _kTtfHeaderBytes = <int>[0, 1, 0, 0, 0, 15, 0, 128, 0, 3, 0, 112];
const String dartPath = '/flutter/dart';
const String constFinderPath = '/flutter/const_finder.snapshot.dart';
const String fontSubsetPath = '/flutter/font-subset';
const String inputPath = '/input/fonts/MaterialIcons-Regular.ttf';
const String outputPath = '/output/fonts/MaterialIcons-Regular.ttf';
const String relativePath = 'fonts/MaterialIcons-Regular.ttf';
List<String> getConstFinderArgs(String appDillPath) => <String>[
dartPath,
constFinderPath,
'--kernel-file', appDillPath,
'--class-library-uri', 'package:flutter/src/widgets/icon_data.dart',
'--class-name', 'IconData',
];
const List<String> fontSubsetArgs = <String>[
fontSubsetPath,
outputPath,
inputPath,
];
void main() {
BufferLogger logger;
MemoryFileSystem fs;
MemoryFileSystem fileSystem;
MockProcessManager mockProcessManager;
MockProcess fontSubsetProcess;
MockArtifacts mockArtifacts;
DevFSStringContent fontManifestContent;
const String dartPath = '/flutter/dart';
const String constFinderPath = '/flutter/const_finder.snapshot.dart';
const String fontSubsetPath = '/flutter/font-subset';
const String inputPath = '/input/fonts/MaterialIcons-Regular.ttf';
const String outputPath = '/output/fonts/MaterialIcons-Regular.ttf';
const String relativePath = 'fonts/MaterialIcons-Regular.ttf';
List<String> getConstFinderArgs(String appDillPath) => <String>[
dartPath,
constFinderPath,
'--kernel-file', appDillPath,
'--class-library-uri', 'package:flutter/src/widgets/icon_data.dart',
'--class-name', 'IconData',
];
const List<String> fontSubsetArgs = <String>[
fontSubsetPath,
outputPath,
inputPath,
];
void _addConstFinderInvocation(
String appDillPath, {
int exitCode = 0,
......@@ -89,19 +90,21 @@ void main() {
mockProcessManager = MockProcessManager();
fontSubsetProcess = MockProcess();
mockArtifacts = MockArtifacts();
fs = MemoryFileSystem();
fileSystem = MemoryFileSystem();
logger = BufferLogger(
terminal: AnsiTerminal(
stdio: mocks.MockStdio(),
platform: _kNoAnsiPlatform,
platform: kNoAnsiPlatform,
),
outputPreferences: OutputPreferences.test(showColor: false),
);
fs.file(constFinderPath).createSync(recursive: true);
fs.file(dartPath).createSync(recursive: true);
fs.file(fontSubsetPath).createSync(recursive: true);
fileSystem.file(constFinderPath).createSync(recursive: true);
fileSystem.file(dartPath).createSync(recursive: true);
fileSystem.file(fontSubsetPath).createSync(recursive: true);
fileSystem.file(inputPath)
..createSync(recursive: true)
..writeAsBytesSync(_kTtfHeaderBytes);
when(mockArtifacts.getArtifactPath(Artifact.constFinder)).thenReturn(constFinderPath);
when(mockArtifacts.getArtifactPath(Artifact.fontSubset)).thenReturn(fontSubsetPath);
when(mockArtifacts.getArtifactPath(Artifact.engineDartBinary)).thenReturn(dartPath);
......@@ -109,11 +112,11 @@ void main() {
Environment _createEnvironment(Map<String, String> defines) {
return Environment.test(
fs.directory('/icon_test')..createSync(recursive: true),
fileSystem.directory('/icon_test')..createSync(recursive: true),
defines: defines,
artifacts: mockArtifacts,
processManager: FakeProcessManager.any(),
fileSystem: fs,
fileSystem: fileSystem,
logger: BufferLogger.test(),
);
}
......@@ -129,18 +132,19 @@ void main() {
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
expect(
logger.errorText,
'Font subetting is not supported in debug mode. The --tree-shake-icons flag will be ignored.\n',
'Font subetting is not supported in debug mode. The --tree-shake-icons'
' flag will be ignored.\n',
);
expect(iconTreeShaker.enabled, false);
final bool subsets = await iconTreeShaker.subsetFont(
inputPath: inputPath,
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
);
......@@ -161,7 +165,7 @@ void main() {
null,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
......@@ -185,7 +189,7 @@ void main() {
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
......@@ -209,13 +213,13 @@ void main() {
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
expect(
() => iconTreeShaker.subsetFont(
inputPath: inputPath,
() async => await iconTreeShaker.subsetFont(
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
),
......@@ -223,20 +227,20 @@ void main() {
);
});
testWithoutContext('The happy path', () async {
testWithoutContext('Can subset a font', () async {
final Environment environment = _createEnvironment(<String, String>{
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')..createSync(recursive: true);
fs.file(inputPath).createSync(recursive: true);
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
final IconTreeShaker iconTreeShaker = IconTreeShaker(
environment,
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
......@@ -245,7 +249,7 @@ void main() {
_resetFontSubsetInvocation(stdinSink: stdinSink);
bool subsetted = await iconTreeShaker.subsetFont(
inputPath: inputPath,
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
);
......@@ -254,7 +258,7 @@ void main() {
expect(subsetted, true);
subsetted = await iconTreeShaker.subsetFont(
inputPath: inputPath,
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
);
......@@ -265,33 +269,104 @@ void main() {
verify(mockProcessManager.start(fontSubsetArgs)).called(2);
});
testWithoutContext('Does not subset a non-ttf font', () async {
final Environment environment = _createEnvironment(<String, String>{
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
final IconTreeShaker iconTreeShaker = IconTreeShaker(
environment,
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
final mocks.CompleterIOSink stdinSink = mocks.CompleterIOSink();
_addConstFinderInvocation(appDill.path, stdout: validConstFinderResult);
_resetFontSubsetInvocation(stdinSink: stdinSink);
final File notAFont = fileSystem.file('input/foo/bar.txt')
..createSync(recursive: true)
..writeAsStringSync('I could not think of a better string');
final bool subsetted = await iconTreeShaker.subsetFont(
input: notAFont,
outputPath: outputPath,
relativePath: relativePath,
);
expect(subsetted, false);
verifyNever(mockProcessManager.run(getConstFinderArgs(appDill.path)));
verifyNever(mockProcessManager.start(fontSubsetArgs));
});
testWithoutContext('Does not subset an invalid ttf font', () async {
final Environment environment = _createEnvironment(<String, String>{
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
final IconTreeShaker iconTreeShaker = IconTreeShaker(
environment,
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
final mocks.CompleterIOSink stdinSink = mocks.CompleterIOSink();
_addConstFinderInvocation(appDill.path, stdout: validConstFinderResult);
_resetFontSubsetInvocation(stdinSink: stdinSink);
final File notAFont = fileSystem.file(inputPath)
..writeAsBytesSync(<int>[0, 1, 2]);
final bool subsetted = await iconTreeShaker.subsetFont(
input: notAFont,
outputPath: outputPath,
relativePath: relativePath,
);
expect(subsetted, false);
verifyNever(mockProcessManager.run(getConstFinderArgs(appDill.path)));
verifyNever(mockProcessManager.start(fontSubsetArgs));
});
testWithoutContext('Non-constant instances', () async {
final Environment environment = _createEnvironment(<String, String>{
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')..createSync(recursive: true);
fs.file(inputPath).createSync(recursive: true);
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
final IconTreeShaker iconTreeShaker = IconTreeShaker(
environment,
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
_addConstFinderInvocation(appDill.path, stdout: constFinderResultWithInvalid);
expect(
iconTreeShaker.subsetFont(
inputPath: inputPath,
await expectLater(
() async => await iconTreeShaker.subsetFont(
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
),
throwsToolExit(
message: 'Avoid non-constant invocations of IconData or try to build again with --no-tree-shake-icons.',
message:
'Avoid non-constant invocations of IconData or try to build'
' again with --no-tree-shake-icons.',
),
);
......@@ -304,15 +379,16 @@ void main() {
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')..createSync(recursive: true);
fs.file(inputPath).createSync(recursive: true);
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
fileSystem.file(inputPath).createSync(recursive: true);
final IconTreeShaker iconTreeShaker = IconTreeShaker(
environment,
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
......@@ -320,9 +396,9 @@ void main() {
_addConstFinderInvocation(appDill.path, stdout: validConstFinderResult);
_resetFontSubsetInvocation(exitCode: -1, stdinSink: stdinSink);
expect(
iconTreeShaker.subsetFont(
inputPath: inputPath,
await expectLater(
() async => await iconTreeShaker.subsetFont(
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
),
......@@ -330,7 +406,7 @@ void main() {
);
verify(mockProcessManager.run(getConstFinderArgs(appDill.path))).called(1);
verifyNever(mockProcessManager.start(fontSubsetArgs));
verify(mockProcessManager.start(fontSubsetArgs)).called(1);
});
testWithoutContext('font-subset throws on write to sdtin', () async {
......@@ -338,15 +414,15 @@ void main() {
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')..createSync(recursive: true);
fs.file(inputPath).createSync(recursive: true);
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
final IconTreeShaker iconTreeShaker = IconTreeShaker(
environment,
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
......@@ -354,9 +430,9 @@ void main() {
_addConstFinderInvocation(appDill.path, stdout: validConstFinderResult);
_resetFontSubsetInvocation(exitCode: -1, stdinSink: stdinSink);
expect(
iconTreeShaker.subsetFont(
inputPath: inputPath,
await expectLater(
() async => await iconTreeShaker.subsetFont(
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
),
......@@ -364,7 +440,7 @@ void main() {
);
verify(mockProcessManager.run(getConstFinderArgs(appDill.path))).called(1);
verifyNever(mockProcessManager.start(fontSubsetArgs));
verify(mockProcessManager.start(fontSubsetArgs)).called(1);
});
testWithoutContext('Invalid font manifest', () async {
......@@ -372,8 +448,8 @@ void main() {
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')..createSync(recursive: true);
fs.file(inputPath).createSync(recursive: true);
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
fontManifestContent = DevFSStringContent(invalidFontManifestJson);
......@@ -382,15 +458,15 @@ void main() {
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
_addConstFinderInvocation(appDill.path, stdout: validConstFinderResult);
expect(
iconTreeShaker.subsetFont(
inputPath: inputPath,
await expectLater(
() async => await iconTreeShaker.subsetFont(
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
),
......@@ -406,8 +482,8 @@ void main() {
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')..createSync(recursive: true);
fs.file(inputPath).createSync(recursive: true);
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
fontManifestContent = DevFSStringContent(invalidFontManifestJson);
......@@ -416,15 +492,15 @@ void main() {
fontManifestContent,
logger: logger,
processManager: mockProcessManager,
fileSystem: fs,
fileSystem: fileSystem,
artifacts: mockArtifacts,
);
_addConstFinderInvocation(appDill.path, exitCode: -1);
expect(
iconTreeShaker.subsetFont(
inputPath: inputPath,
await expectLater(
() async => await iconTreeShaker.subsetFont(
input: fileSystem.file(inputPath),
outputPath: outputPath,
relativePath: relativePath,
),
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment