Skip to content

Commit

Permalink
Use fixed-length List where possible.
Browse files Browse the repository at this point in the history 
In short:
Heap size: -155 MB, about 9%
Object count: -4.6 * 10^6, about 20%


rwf-materials/01-setting-up-your-environment

=== Before.

[Analysis contexts: 35]
[time: 16521 ms]
  [26802 ms] Created HeapSnapshotGraph.
  externalSize: 1984
  shallowSize: 1757489376
  Objects: 23454755
  [26803 ms] Created Analysis.
All objects.
size        count     class
--------    --------  --------
278670 kb   2110088   _OneByteString dart:core
270497 kb   2090817   _List dart:core
229974 kb   2066512   _Uint32List dart:typed_data
154250 kb   4936008   _GrowableList dart:core
 63129 kb   160607    _Uint8List dart:typed_data
 56224 kb   1199459   Reference package:analyzer/src/summary2/reference.dart
 42643 kb   682297    _InfoMethodDeclaration package:analyzer/src/summary2/informative_data.dart
 40358 kb   234812    PropertyAccessorElementImpl_ImplicitGetter package:analyzer/src/dart/element/element.dart
 38075 kb   203067    ConstFieldElementImpl package:analyzer/src/dart/element/element.dart
 32181 kb   686528    _InfoFormalParameter package:analyzer/src/summary2/informative_data.dart
 29730 kb   475685    _Closure dart:core
 27724 kb     6868    _TwoByteString dart:core
 25594 kb   409516    _Map dart:collection
 17778 kb    81273    ClassElementImpl package:analyzer/src/dart/element/element.dart
 16479 kb   411894    Context
 14672 kb    93903    LibraryImportElementImpl package:analyzer/src/dart/element/element.dart
 14074 kb    90077    ParameterElementImpl package:analyzer/src/dart/element/element.dart
 13316 kb   284078    _InfoFieldDeclaration package:analyzer/src/summary2/informative_data.dart
 13138 kb    19582    Instructions
 11985 kb   191772    InterfaceTypeImpl package:analyzer/src/dart/element/type.dart
--------    --------
1701770 kb  23237353

Instances of: _GrowableList
size       count     class
--------   --------  --------
154250 kb  4936008   _GrowableList dart:core

Instances of empty: _GrowableList
size       count     class
--------   --------  --------
119542 kb  3825345   _GrowableList dart:core



=== After

[Analysis contexts: 35]
[time: 16946 ms]
  [24964 ms] Created HeapSnapshotGraph.
  externalSize: 1984
  shallowSize: 1611104288
  Objects: 18819706
  [24965 ms] Created Analysis.
All objects.
size        count     class
--------    --------  --------
278679 kb   2110125   _OneByteString dart:core
265723 kb   2097564   _List dart:core
229974 kb   2066512   _Uint32List dart:typed_data
 62996 kb   158742    _Uint8List dart:typed_data
 56224 kb   1199459   Reference package:analyzer/src/summary2/reference.dart
 42643 kb   682297    _InfoMethodDeclaration package:analyzer/src/summary2/informative_data.dart
 40358 kb   234812    PropertyAccessorElementImpl_ImplicitGetter package:analyzer/src/dart/element/element.dart
 38075 kb   203067    ConstFieldElementImpl package:analyzer/src/dart/element/element.dart
 32181 kb   686528    _InfoFormalParameter package:analyzer/src/summary2/informative_data.dart
 29730 kb   475685    _Closure dart:core
 27724 kb     6868    _TwoByteString dart:core
 25594 kb   409516    _Map dart:collection
 17778 kb    81273    ClassElementImpl package:analyzer/src/dart/element/element.dart
 16479 kb   411894    Context
 14672 kb    93903    LibraryImportElementImpl package:analyzer/src/dart/element/element.dart
 14074 kb    90077    ParameterElementImpl package:analyzer/src/dart/element/element.dart
 13316 kb   284078    _InfoFieldDeclaration package:analyzer/src/summary2/informative_data.dart
 13117 kb    19633    Instructions
 11985 kb   191772    InterfaceTypeImpl package:analyzer/src/dart/element/type.dart
 11520 kb   184326    FieldElementLinkedData package:analyzer/src/summary2/bundle_reader.dart
--------    --------
1546419 kb  18428158

Instances of: LibraryElementImpl
size       count     class
--------   --------  --------
  4491 kb   15129    LibraryElementImpl package:analyzer/src/dart/element/element.dart

Instances of: _GrowableList
size       count     class
--------   --------  --------
  3795 kb  121445    _GrowableList dart:core

Instances of empty: _GrowableList
size       count     class
--------   --------  --------
  2075 kb   66426    _GrowableList dart:core


Change-Id: I4ed72298a2361e9fe004bef5482a9f8bae5de6df
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/279644
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
  • Loading branch information
@scheglov
scheglov authored and Commit Queue committed Jan 27, 2023
1 parent bb63927 commit b4640c3
Show file tree
Hide file tree
Showing 31 changed files with 326 additions and 231 deletions.
2 changes: 1 addition & 1 deletion pkg/analyzer/lib/src/dart/analysis/driver.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ import 'package:analyzer/src/utilities/uri_cache.dart';
/// TODO(scheglov) Clean up the list of implicitly analyzed files.
class AnalysisDriver implements AnalysisDriverGeneric {
/// The version of data format, should be incremented on every format change.
static const int DATA_VERSION = 256;
static const int DATA_VERSION = 257;

/// The number of exception contexts allowed to write. Once this field is
/// zero, we stop writing any new exception contexts in this process.
Expand Down
41 changes: 21 additions & 20 deletions pkg/analyzer/lib/src/dart/analysis/file_state.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import 'package:analyzer/src/util/either.dart';
import 'package:analyzer/src/util/file_paths.dart' as file_paths;
import 'package:analyzer/src/util/performance/operation_performance.dart';
import 'package:analyzer/src/util/uri.dart';
import 'package:analyzer/src/utilities/extensions/collection.dart';
import 'package:analyzer/src/utilities/uri_cache.dart';
import 'package:analyzer/src/workspace/workspace.dart';
import 'package:collection/collection.dart';
Expand Down Expand Up @@ -608,18 +609,18 @@ class FileState {
) {
final primaryUri = _buildDirectiveUri(directive.uri);

final configurationUris = <DirectiveUri>[];
DirectiveUri? selectedConfigurationUri;
for (final configuration in directive.configurations) {
final configurationUris = directive.configurations.map((configuration) {
final configurationUri = _buildDirectiveUri(configuration.uri);
configurationUris.add(configurationUri);
// Maybe select this URI.
final name = configuration.name;
final value = configuration.valueOrTrue;
if (_fsState._declaredVariables.get(name) == value) {
selectedConfigurationUri ??= configurationUri;
}
}
// Include it anyway.
return configurationUri;
}).toFixedList();

return NamespaceDirectiveUris(
primary: primaryUri,
Expand Down Expand Up @@ -929,7 +930,7 @@ class FileState {
.whereType<ConstructorDeclaration>()
.map((e) => e.name?.lexeme ?? '')
.where((e) => !e.startsWith('_'))
.toList();
.toFixedList();
if (constructors.isNotEmpty) {
macroClasses.add(
MacroClass(
Expand All @@ -944,8 +945,8 @@ class FileState {
if (!isDartCore && !hasDartCoreImport) {
imports.add(
UnlinkedLibraryImportDirective(
combinators: [],
configurations: [],
combinators: const [],
configurations: const [],
importKeywordOffset: -1,
isSyntheticDartCore: true,
prefix: null,
Expand Down Expand Up @@ -978,15 +979,15 @@ class FileState {

return UnlinkedUnit(
apiSignature: Uint8List.fromList(computeUnlinkedApiSignature(unit)),
augmentations: augmentations,
exports: exports,
imports: imports,
augmentations: augmentations.toFixedList(),
exports: exports.toFixedList(),
imports: imports.toFixedList(),
informativeBytes: writeUnitInformative(unit),
libraryAugmentationDirective: libraryAugmentationDirective,
libraryDirective: libraryDirective,
lineStarts: Uint32List.fromList(unit.lineInfo.lineStarts),
macroClasses: macroClasses,
parts: parts,
macroClasses: macroClasses.toFixedList(),
parts: parts.toFixedList(),
partOfNameDirective: partOfNameDirective,
partOfUriDirective: partOfUriDirective,
topLevelDeclarations: topLevelDeclarations,
Expand Down Expand Up @@ -1020,18 +1021,18 @@ class FileState {
keywordOffset: combinator.keyword.offset,
endOffset: combinator.end,
isShow: true,
names: combinator.shownNames.map((e) => e.name).toList(),
names: combinator.shownNames.map((e) => e.name).toFixedList(),
);
} else {
combinator as HideCombinator;
return UnlinkedCombinator(
keywordOffset: combinator.keyword.offset,
endOffset: combinator.end,
isShow: false,
names: combinator.hiddenNames.map((e) => e.name).toList(),
names: combinator.hiddenNames.map((e) => e.name).toFixedList(),
);
}
}).toList();
}).toFixedList();
}

static List<UnlinkedNamespaceDirectiveConfiguration> _serializeConfigurations(
Expand All @@ -1045,7 +1046,7 @@ class FileState {
value: value,
uri: configuration.uri.stringValue,
);
}).toList();
}).toFixedList();
}

static UnlinkedLibraryExportDirective _serializeExport(ExportDirective node) {
Expand Down Expand Up @@ -1748,7 +1749,7 @@ class LibraryFileKind extends LibraryOrAugmentationFileKind {
uri: uri,
);
}
}).toList();
}).toFixedList();
}

@override
Expand Down Expand Up @@ -1940,7 +1941,7 @@ abstract class LibraryOrAugmentationFileKind extends FileKind {
uri: uri,
);
}
}).toList();
}).toFixedList();
}

List<LibraryExportState> get libraryExports {
Expand Down Expand Up @@ -1980,7 +1981,7 @@ abstract class LibraryOrAugmentationFileKind extends FileKind {
uris: uris,
);
}
}).toList();
}).toFixedList();
}

List<LibraryImportState> get libraryImports {
Expand Down Expand Up @@ -2020,7 +2021,7 @@ abstract class LibraryOrAugmentationFileKind extends FileKind {
uris: uris,
);
}
}).toList();
}).toFixedList();
}

/// Collect files that are transitively referenced by this library.
Expand Down
3 changes: 2 additions & 1 deletion pkg/analyzer/lib/src/dart/analysis/library_graph.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'package:_fe_analyzer_shared/src/util/dependency_walker.dart' as graph
show DependencyWalker, Node;
import 'package:analyzer/src/dart/analysis/file_state.dart';
import 'package:analyzer/src/summary/api_signature.dart';
import 'package:analyzer/src/utilities/extensions/collection.dart';
import 'package:collection/collection.dart';

/// Ensure that the [FileState.libraryCycle] for the [file] and anything it
Expand Down Expand Up @@ -223,7 +224,7 @@ class _LibraryWalker extends graph.DependencyWalker<_LibraryNode> {

// Create the LibraryCycle instance for the cycle.
var cycle = LibraryCycle(
libraries: libraries,
libraries: libraries.toFixedList(),
directDependencies: directDependencies,
apiSignature: apiSignature.toHex(),
implSignature: implSignature.toHex(),
Expand Down
12 changes: 7 additions & 5 deletions pkg/analyzer/lib/src/dart/constant/evaluation.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import 'package:analyzer/src/error/codes.dart';
import 'package:analyzer/src/generated/constant.dart';
import 'package:analyzer/src/generated/engine.dart';
import 'package:analyzer/src/task/api/model.dart';
import 'package:analyzer/src/utilities/extensions/collection.dart';

class ConstantEvaluationConfiguration {
/// During evaluation of enum constants we might need to report an error
Expand Down Expand Up @@ -1367,7 +1368,7 @@ class ConstantVisitor extends UnifyingAstVisitor<DartObjectImpl> {
var type = variableElement.instantiate(
typeArguments: variableElement.typeParameters
.map((t) => _typeProvider.dynamicType)
.toList(),
.toFixedList(),
nullabilitySuffix: NullabilitySuffix.star,
);
return DartObjectImpl(
Expand Down Expand Up @@ -2038,15 +2039,16 @@ class EvaluationResultImpl {
/// The errors encountered while trying to evaluate the compile time constant.
/// These errors may or may not have prevented the expression from being a
/// valid compile time constant.
late final List<AnalysisError> _errors;
final List<AnalysisError> _errors;

/// The value of the expression, or `null` if the value couldn't be computed
/// due to errors.
final DartObjectImpl? value;

EvaluationResultImpl(this.value, [List<AnalysisError>? errors]) {
_errors = errors ?? <AnalysisError>[];
}
EvaluationResultImpl(
this.value, [
this._errors = const [],
]);

List<AnalysisError> get errors => _errors;

Expand Down
5 changes: 3 additions & 2 deletions pkg/analyzer/lib/src/dart/element/class_hierarchy.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/src/dart/element/element.dart';
import 'package:analyzer/src/dart/element/type_algebra.dart';
import 'package:analyzer/src/dart/element/type_system.dart';
import 'package:analyzer/src/utilities/extensions/collection.dart';

class ClassHierarchy {
final Map<InterfaceElement, _Hierarchy> _map = {};
Expand Down Expand Up @@ -88,8 +89,8 @@ class ClassHierarchy {
interfaces.add(collector.type);
}

hierarchy.errors = errors;
hierarchy.interfaces = interfaces;
hierarchy.errors = errors.toFixedList();
hierarchy.interfaces = interfaces.toFixedList();

return hierarchy;
}
Expand Down
21 changes: 13 additions & 8 deletions pkg/analyzer/lib/src/dart/element/element.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import 'package:analyzer/src/summary2/macro_application_error.dart';
import 'package:analyzer/src/summary2/reference.dart';
import 'package:analyzer/src/task/inference_error.dart';
import 'package:analyzer/src/util/file_paths.dart' as file_paths;
import 'package:analyzer/src/utilities/extensions/collection.dart';
import 'package:analyzer/src/utilities/extensions/string.dart';
import 'package:collection/collection.dart';

Expand Down Expand Up @@ -207,7 +208,7 @@ abstract class AbstractClassElementImpl extends _ExistingElementImpl
if (typeParameters.isNotEmpty) {
typeArguments = typeParameters.map<DartType>((t) {
return t.instantiate(nullabilitySuffix: _noneOrStarSuffix);
}).toList();
}).toFixedList();
} else {
typeArguments = const <DartType>[];
}
Expand Down Expand Up @@ -922,7 +923,7 @@ class ClassElementImpl extends ClassOrMixinElementImpl implements ClassElement {
..staticType = implicitParameter.type,
);
}
implicitConstructor.parameters = implicitParameters;
implicitConstructor.parameters = implicitParameters.toFixedList();
}
implicitConstructor.enclosingElement = this;
// TODO(scheglov) Why do we manually map parameters types above?
Expand Down Expand Up @@ -2874,7 +2875,7 @@ class ElementLocationImpl implements ElementLocation {
components.insert(0, (ancestor as ElementImpl).identifier);
ancestor = ancestor.enclosingElement;
}
_components = components;
_components = components.toFixedList();
}

/// Initialize a newly created location from the given [encoding].
Expand Down Expand Up @@ -4394,7 +4395,7 @@ class LibraryElementImpl extends LibraryOrAugmentationElementImpl
}

visitAugmentations(this);
return result;
return result.toFixedList();
}

@override
Expand Down Expand Up @@ -4709,7 +4710,11 @@ class LocalVariableElementImpl extends NonParameterVariableElementImpl

mixin MacroTargetElement {
/// Errors registered while applying macros to this element.
List<MacroApplicationError> macroApplicationErrors = [];
List<MacroApplicationError> macroApplicationErrors = const [];

void addMacroApplicationError(MacroApplicationError error) {
macroApplicationErrors = [...macroApplicationErrors, error];
}
}

/// Marker interface for elements that may have [MacroTargetElement]s.
Expand Down Expand Up @@ -5869,9 +5874,9 @@ class PropertyAccessorElementImpl_ImplicitSetter
return _parameters;
}

return _parameters = <ParameterElement>[
ParameterElementImpl_ofImplicitSetter(this)
];
return _parameters = List.generate(
1, (_) => ParameterElementImpl_ofImplicitSetter(this),
growable: false);
}

@override
Expand Down
8 changes: 6 additions & 2 deletions pkg/analyzer/lib/src/dart/element/inheritance_manager3.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:analyzer/src/dart/element/extensions.dart';
import 'package:analyzer/src/dart/element/member.dart';
import 'package:analyzer/src/dart/element/type_algebra.dart';
import 'package:analyzer/src/dart/element/type_system.dart';
import 'package:analyzer/src/utilities/extensions/collection.dart';

/// Failure because of there is no most specific signature in [candidates].
class CandidatesConflict extends Conflict {
Expand Down Expand Up @@ -642,7 +643,7 @@ class InheritanceManager3 {
noSuchMethodForwarders,
namedCandidates,
superImplemented,
conflicts,
conflicts.toFixedList(),
);
}

Expand Down Expand Up @@ -702,7 +703,10 @@ class InheritanceManager3 {
{},
interfaceCandidates,
[superInterface],
<Conflict>[...superConflicts, ...interfaceConflicts],
<Conflict>[
...superConflicts,
...interfaceConflicts,
].toFixedList(),
);
}

Expand Down
9 changes: 5 additions & 4 deletions pkg/analyzer/lib/src/dart/element/normalize.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:analyzer/src/dart/element/type.dart';
import 'package:analyzer/src/dart/element/type_algebra.dart';
import 'package:analyzer/src/dart/element/type_provider.dart';
import 'package:analyzer/src/dart/element/type_system.dart';
import 'package:analyzer/src/utilities/extensions/collection.dart';

/// Helper for computing canonical presentation of types.
///
Expand Down Expand Up @@ -50,7 +51,7 @@ class NormalizeHelper {
return e.copyWith(
type: _normalize(e.type),
);
}).toList(),
}).toFixedList(),
returnType: _normalize(functionType.returnType),
nullabilitySuffix: NullabilitySuffix.none,
);
Expand Down Expand Up @@ -140,7 +141,7 @@ class NormalizeHelper {
// NORM(C<T0, ..., Tn>) = C<R0, ..., Rn> where Ri is NORM(Ti)
if (T is InterfaceType) {
return T.element.instantiate(
typeArguments: T.typeArguments.map(_normalize).toList(),
typeArguments: T.typeArguments.map(_normalize).toFixedList(),
nullabilitySuffix: NullabilitySuffix.none,
);
}
Expand All @@ -152,13 +153,13 @@ class NormalizeHelper {
return RecordTypePositionalFieldImpl(
type: _normalize(field.type),
);
}).toList(),
}).toFixedList(),
namedFields: T.namedFields.map((field) {
return RecordTypeNamedFieldImpl(
name: field.name,
type: _normalize(field.type),
);
}).toList(),
}).toFixedList(),
nullabilitySuffix: NullabilitySuffix.none,
);
}
Expand Down
7 changes: 5 additions & 2 deletions pkg/analyzer/lib/src/dart/element/scope.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:analyzer/dart/element/scope.dart';
import 'package:analyzer/src/dart/element/element.dart';
import 'package:analyzer/src/summary2/combinator.dart';
import 'package:analyzer/src/summary2/export.dart';
import 'package:analyzer/src/utilities/extensions/collection.dart';

/// The scope for the initializers in a constructor.
class ConstructorInitializerScope extends EnclosedScope {
Expand Down Expand Up @@ -97,7 +98,7 @@ class InterfaceScope extends EnclosedScope {

class LibraryOrAugmentationScope extends EnclosedScope {
final LibraryOrAugmentationElementImpl _container;
final List<ExtensionElement> extensions = [];
List<ExtensionElement> extensions = [];

LibraryOrAugmentationScope(LibraryOrAugmentationElementImpl container)
: _container = container,
Expand All @@ -116,6 +117,8 @@ class LibraryOrAugmentationScope extends EnclosedScope {
_addGetter(DynamicElementImpl.instance);
_addGetter(NeverElementImpl.instance);
}

extensions = extensions.toFixedList();
}

void _addExtension(ExtensionElement element) {
Expand Down Expand Up @@ -383,7 +386,7 @@ class _LibraryOrAugmentationImportScope implements Scope {
..._nullPrefixScope._extensions,
for (var prefix in _container.prefixes)
...(prefix.scope as PrefixScope)._extensions,
}.toList();
}.toFixedList();
}

@override
Expand Down
Loading

4 comments on commit b4640c3

@bernaferrari
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this, but it's weird to me the fixed list is of type list. Won't this create issues in maintenance in the future, like const a = []; a.add(1) is invalid? Is there anything we can make to make the development experience better/more explicit in this regard?

@scheglov
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ideally we would need separate types for immutable collections, and mutable collections.
You are right, that the absence of the type-level separation makes it impossible to know if such code will fail at runtime.
But we (in the analyzer) are limited by the language capabilities.

@bernaferrari
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @munificent pleaseeee

@munificent
Copy link
Member

Choose a reason for hiding this comment

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

Waaaay back before Dart 1.0 when the collections API was first designed, the folks working on it spent a lot of time discussing whether Dart should expose separate read-only and read-write APIs for collections. That way, if you had an immutable list, the API for the list wouldn't even let you try to call mutating methods on it.

They decided to not do that. Their argument was that it kept the API simpler (which it does). We sort of have it for lists with Iterable and List. But Iterable doesn't have operator []. (Though it does have elementAt().)

We could conceivably add a new read-only list (and map and set) interface, but given how close Iterable already is to that, it would probably just add confusion.

In practice, users want to distinguish between:

  • Lists that are mutable but fixed-length (this change). Fixed-length lists are faster and use less memory.
  • Lists that are immutable in that no code anywhere can add, remove, or change list elements.
  • Lists that are deeply immutable and it's known that the elements are immutable too.
  • Lists that are read-only where some code is prohibited from modifying it but other code can't (like const in C++).

Deciding which of those properties are visible in the type system and API design (and thus statically checked) is a Hard Problem.

Please sign in to comment.