X Tutup
Skip to content

perf(dart/transform): Only process deferred libs when necessary #6745

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.

Sign up for GitHub

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

Closed
kegluneq wants to merge 1 commit into from
Closed

perf(dart/transform): Only process deferred libs when necessary #6745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion modules_dart/transform/lib/src/transform/common/names.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const SETUP_METHOD_NAME = 'initReflector';
const REFLECTOR_VAR_NAME = 'reflector';
const TRANSFORM_DYNAMIC_MODE = 'transform_dynamic';
const CSS_EXTENSION = '.css';
const DEFERRED_EXTENSION = '.dart.deferredCount';
const SHIMMED_STYLESHEET_EXTENSION = '.css.shim.dart';
const NON_SHIMMED_STYLESHEET_EXTENSION = '.css.dart';
const META_EXTENSION = '.ng_meta.json';
Expand All @@ -21,8 +22,10 @@ const TEMPLATE_EXTENSION = '.template.dart';

/// Note that due to the implementation of `_toExtension`, ordering is
/// important. For example, putting '.dart' first in this list will cause
/// incorrect behavior.
/// incorrect behavior because it will (incompletely) match '.template.dart'
/// files.
const ALL_EXTENSIONS = const [
DEFERRED_EXTENSION,
META_EXTENSION,
SUMMARY_META_EXTENSION,
TEMPLATE_EXTENSION,
Expand All @@ -36,6 +39,7 @@ const ALL_EXTENSIONS = const [
/// any files named like transformer outputs will be reported as generated.
bool isGenerated(String uri) {
return const [
DEFERRED_EXTENSION,
META_EXTENSION,
NON_SHIMMED_STYLESHEET_EXTENSION,
SHIMMED_STYLESHEET_EXTENSION,
Expand All @@ -44,6 +48,10 @@ bool isGenerated(String uri) {
].any((ext) => uri.endsWith(ext));
}

/// Returns `uri` with its extension updated to [DEFERRED_EXTENSION].
String toDeferredExtension(String uri) =>
_toExtension(uri, ALL_EXTENSIONS, DEFERRED_EXTENSION);
Copy link
Copy Markdown
Contributor

@jakemac53 jakemac53 Jan 28, 2016

Choose a reason for hiding this comment

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

Is ALL_EXTENSIONS appropriate here? I thought this would be only const ['.dart'].

Copy link
Copy Markdown
Author

@kegluneq kegluneq Jan 28, 2016

Choose a reason for hiding this comment

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

I thought it would be confusing to have the behavior of toDeferredExtension be different from the other to*Extension methods in this file.

Copy link
Copy Markdown
Contributor

@jakemac53 jakemac53 Jan 28, 2016

Choose a reason for hiding this comment

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

Well, toShimmedStylesheetExtension and toNonShimmedStylesheetExtension use const [CSS_EXTENSION].

Maybe these all really need to be updated, and should assert that the uri passed in matches the expected extension?

Copy link
Copy Markdown
Author

@kegluneq kegluneq Jan 28, 2016

Choose a reason for hiding this comment

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

I think of CSS resources as being in a different bucket -- they don't interact with .dart files and vice versa.

What would be the benefit of such a change? I'm against doing the work for performance reasons (unless we can demonstrate that they're actually hurting us). If these are hurting readability, I'm not opposed. It would require searching through the transformers to find wherever these are used & determine the expected extensions and then require updates if we ever want to use them with a new "from" extension, so I don't want to do the work unless there's a clear benefit.

Copy link
Copy Markdown
Author

@kegluneq kegluneq Jan 28, 2016

Choose a reason for hiding this comment

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

Update to to*Extension functions in #6753


/// Returns `uri` with its extension updated to [META_EXTENSION].
String toMetaExtension(String uri) =>
_toExtension(uri, ALL_EXTENSIONS, META_EXTENSION);
Expand Down
57 changes: 47 additions & 10 deletions modules_dart/transform/lib/src/transform/deferred_rewriter/transformer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import 'rewriter.dart';
/// Transformer responsible for rewriting deferred library loads to enable
/// initializing the reflector in a deferred way to keep the code with the
/// deferred library.
class DeferredRewriter extends Transformer implements LazyTransformer {
class DeferredRewriter extends AggregateTransformer implements LazyTransformer {
Copy link
Copy Markdown
Contributor

@jakemac53 jakemac53 Jan 28, 2016

Choose a reason for hiding this comment

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

Not sure why this needs to be an AggregateTransformer? Instead all non-generated .dart files could be treated as primary inputs, and then _assetToProcess would simply check for the existence of a .dart.hasDeferred file. (I am also not sure why we care about the count?)

Copy link
Copy Markdown
Contributor

@jakemac53 jakemac53 Jan 28, 2016

Choose a reason for hiding this comment

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

Or is it just so you can consume the file?

Copy link
Copy Markdown
Contributor

@jakemac53 jakemac53 Jan 28, 2016

Choose a reason for hiding this comment

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

Actually, even better you could only mark .dart.hasDeferred files as primary, and then read in the corresponding .dart files? Or can you not overwrite those files if they aren't primary inputs (I can't remember for sure).

Copy link
Copy Markdown
Author

@kegluneq kegluneq Jan 28, 2016

Choose a reason for hiding this comment

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

Yes :)

  • Only primary assets can be consumed
  • Only primary assets can be overwritten

So I could have both be primaries and have them interact in such a way that if the .dart.deferredCount was present, then the .dart file would "consume itself", and the .dart.deferredCount would output an associated .dart file, but I like the AggregateTransform strategy better.

Copy link
Copy Markdown
Contributor

@jakemac53 jakemac53 Jan 28, 2016

Choose a reason for hiding this comment

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

Ya ok, it seems a bit more complicated than it needs to be but I can't really think of a way to simplify it either :(.

I do still think that the deferredCount file should just become an empty hasDeferred file, and then you will never have to read the contents of the file. I guess it only ever exists as an in memory asset anyways though, so maybe it doesn't matter.

final TransformerOptions options;

DeferredRewriter(this.options);
Expand All @@ -25,23 +25,60 @@ class DeferredRewriter extends Transformer implements LazyTransformer {
}

@override
bool isPrimary(AssetId id) =>
id.extension.endsWith('dart') && _isNotGenerated(id);
dynamic classifyPrimary(AssetId id) {
// Map <name>.dart and <name>.dart.deferredCount => <name>.
// Anything else to `null`.
var extension = null;
if (id.path.endsWith(DEFERRED_EXTENSION)) {
extension = DEFERRED_EXTENSION;
} else if (id.path.endsWith('.dart') && !isGenerated(id.path)) {
extension = '.dart';
} else {
return null;
}
return id.path.substring(0, id.path.length - extension.length);
}

@override
Future apply(Transform transform) async {
Future apply(AggregateTransform transform) async {
return zone.exec(() async {
var asset = transform.primaryInput;
var reader = new AssetReader.fromTransform(transform);
var transformedCode = await rewriteDeferredLibraries(reader, asset.id);
final dartAsset = await _assetToProcess(transform);
if (dartAsset == null) return;

var transformedCode = await rewriteDeferredLibraries(
new AssetReader.fromTransform(transform), dartAsset.id);
if (transformedCode != null) {
transform.addOutput(new Asset.fromString(asset.id, transformedCode));
transform
.addOutput(new Asset.fromString(dartAsset.id, transformedCode));
}
}, log: transform.logger);
}
}

bool _isNotGenerated(AssetId id) => !isGenerated(id.path);
/// Returns the asset we need to process or `null` if none exists.
///
/// Consumes the .dart.deferredCount asset if it is present.
Future<Asset> _assetToProcess(AggregateTransform transform) async {
// We only need to process .dart files that have an associated
// .dart.deferredCount file with a value != "0".
//
// The .dart.deferredCount file is generated by a previous phase for files
// which have deferred imports. An absent .dart.deferredCount asset is the
// treated the same as a .dart.deferredCount asset with value "0".
var dartAsset, deferredCountAsset;
await for (Asset a in transform.primaryInputs) {
if (a.id.path.endsWith(DEFERRED_EXTENSION)) {
deferredCountAsset = a;
} else if (a.id.path.endsWith('.dart')) {
dartAsset = a;
}
}
if (deferredCountAsset == null) return null;
// No longer necessary.
transform.consumePrimary(deferredCountAsset.id);
if ((await deferredCountAsset.readAsString()) == "0") return null;
return dartAsset;
}
}

// Visible for testing
Future<String> rewriteDeferredLibraries(AssetReader reader, AssetId id) async {
Expand Down
17 changes: 17 additions & 0 deletions modules_dart/transform/lib/src/transform/directive_processor/transformer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class DirectiveProcessor extends Transformer implements LazyTransformer {

@override
declareOutputs(DeclaringTransform transform) {
transform.declareOutput(_deferredAssetId(transform.primaryId));
transform.declareOutput(_ngSummaryAssetId(transform.primaryId));
}

Expand All @@ -49,6 +50,17 @@ class DirectiveProcessor extends Transformer implements LazyTransformer {
}
transform.addOutput(new Asset.fromString(
_ngSummaryAssetId(primaryId), _encoder.convert(ngMeta.toJson())));

var deferredCount = 0;
if (ngMeta.ngDeps != null) {
deferredCount = ngMeta.ngDeps.imports.where((i) => i.isDeferred).length;
}
if (deferredCount > 0) {
// The existence of this file with the value != "0" signals
// DeferredRewriter that the associated .dart file needs attention.
transform.addOutput(new Asset.fromString(
_deferredAssetId(primaryId), deferredCount.toString()));
}
}, log: transform.logger);
}
}
Expand All @@ -57,3 +69,8 @@ AssetId _ngSummaryAssetId(AssetId primaryInputId) {
return new AssetId(
primaryInputId.package, toSummaryExtension(primaryInputId.path));
}

AssetId _deferredAssetId(AssetId primaryInputId) {
return new AssetId(
primaryInputId.package, toDeferredExtension(primaryInputId.path));
}
X Tutup