This is a reland of commit b9e25afcb4
Original change's description:
> [dart2js] Stop emitting a dill with closed world.
>
> All transformations to the dill are now performed with the CFE linker phase (phase0b). This means we no longer need to emit a dill as part of the closed world computation. This saves both time and memory resources associated with the cost of serialization during that phase.
>
> Change-Id: I28dde8a1eecd71e823880027505f62fc804fcca6
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332821
> Reviewed-by: Sigmund Cherem <sigmund@google.com>
> Reviewed-by: Mayank Patke <fishythefish@google.com>
> Commit-Queue: Nate Biggs <natebiggs@google.com>
Change-Id: Ia693213dcdc949a8ef4b4204cc033130274d2381
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336982
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
Simplify the js_ast printer - it should not transform the program
while printing.
js_ast:
- Don't skip blocks containing a single statement.
- Don't rewrite arrow functions.
dart2js:
- Fix rewrite_async.dart to avoid a few singleton blocks.
- Static initializer thunks are transformed in arrow functions with expression bodies.
Change-Id: I4ab75c6ca7f580835a6c9d4eb45240d76df9d895
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336820
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Nate Biggs <natebiggs@google.com>
This is a reland of commit bebd08746b
Original change's description:
> [dart2js] Evaluate CFE consts as part of phase 0b (CFE linker).
>
> Constants are current evaluated in a few places during closed world generation, primarily as part of the ScopeModelBuilder. The scope visitor was modifying the AST which meant we had to emit a new dill with these evaluated constants along with the closed world results.
>
> This change instead evaluates the constants directly after linking the Kernel as part of the global transformations. This means we can update the ScopeModelBuilder to not mutate the AST at all as all constants are already simplified.
>
> A potential follow up here is to simplify the ScopeModelBuilder since all nodes should already be simplified if they can be, we should be able to avoid visiting some children.
>
> After this change we only directly create a single ConstantEvaluator, the one in `load_kernel`. The const simplifier also creates one and a follow up CL moves this to to run right after this new transformation.
>
> Note: Alternate versions of this CL tried to make the global transformation simpler by either:
> 1) Running the const evaluator indiscriminately on all expressions. This didn't work because it lead to exponential computation on constants set up as a DAG (see tests/language/const/constant_dag_test).
> 2) Only evaluating ConstantExpression nodes to update UnevaluatedConstants. This does not cover all the cases where the ScopeModelBuilder is modifying the tree and lead to a different compiler output.
>
> Change-Id: I746d889b37feddc9ab6c386c6252016dec745e6e
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332601
> Reviewed-by: Mayank Patke <fishythefish@google.com>
Change-Id: I53871a57144a3a1bd363af141d8f31c8ffa5ca6d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336221
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
All transformations to the dill are now performed with the CFE linker phase (phase0b). This means we no longer need to emit a dill as part of the closed world computation. This saves both time and memory resources associated with the cost of serialization during that phase.
Change-Id: I28dde8a1eecd71e823880027505f62fc804fcca6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332821
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
Constants are current evaluated in a few places during closed world generation, primarily as part of the ScopeModelBuilder. The scope visitor was modifying the AST which meant we had to emit a new dill with these evaluated constants along with the closed world results.
This change instead evaluates the constants directly after linking the Kernel as part of the global transformations. This means we can update the ScopeModelBuilder to not mutate the AST at all as all constants are already simplified.
A potential follow up here is to simplify the ScopeModelBuilder since all nodes should already be simplified if they can be, we should be able to avoid visiting some children.
After this change we only directly create a single ConstantEvaluator, the one in `load_kernel`. The const simplifier also creates one and a follow up CL moves this to to run right after this new transformation.
Note: Alternate versions of this CL tried to make the global transformation simpler by either:
1) Running the const evaluator indiscriminately on all expressions. This didn't work because it lead to exponential computation on constants set up as a DAG (see tests/language/const/constant_dag_test).
2) Only evaluating ConstantExpression nodes to update UnevaluatedConstants. This does not cover all the cases where the ScopeModelBuilder is modifying the tree and lead to a different compiler output.
Change-Id: I746d889b37feddc9ab6c386c6252016dec745e6e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332601
Reviewed-by: Mayank Patke <fishythefish@google.com>
I suspect that when the list of type preserving selectors was created the first/last setters either didn't exist or were just overlooked. Any dynamic List could have its inferred type changed by these operations.
Due to Dart2js's type representation this bug affects more than just dynamic lists. We track some simple values as part of the type system. So an operation that modifies a value can technically modify the type, as Dart2js represents it, even if the "real" type is preserved.
In the added test we would represent the list literal's type as "List(length: 2, elementType: Bool(true))". Notice the type states the elementType is specifically true, not just bool. Thus the first/last operations are modifying that type. But since we aren't registering this type change, SSA optimizes away the call/index and inlines the element itself.
Interestingly, this primarily manifested for bools due to a check in SSA:
https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/ssa/optimize.dart#L475
In that code we are inlining constant-like expressions for the arguments of static invocations (such as the argument to a Expect.isTrue call). However, a few lines earlier you will see we only inline bool constants:
https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/ssa/optimize.dart#L467
So when the list element type is anything other than a bool, this inlining does not trigger thus avoiding the bug.
Bug: https://github.com/dart-lang/sdk/issues/53944
Change-Id: I4e893903f335fc99b13cf526736c27bb066a4bad
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/334420
Reviewed-by: Stephen Adams <sra@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
The Dart2JS team has agreed that this mode of compilation is no longer worth investing in at this time and its existence adds some overhead to other feature work so it is worth fully removing. In the future we may revisit this mode of compilation. Below is some more context for any future exploration in this space.
What didn't work with modular analysis:
- current modular analysis was computing impacts, which were dense (50% of the size of kernel)
- using it moved work to a modular phase, and cut Phase1 in half however end-to-end time was not better
- data overhead was very high
- it made it much harder to maintain invariants throughout the pipeline: the data is tightly coupled with the kernel AST, making it hard to make late modifications to the AST.
How to potentially make it better:
- make the data much more sparse
- make the data more independent from the kernel AST so that transformations are not breaking
- reduce the critical path in a more substantial way.
Note: We retain and ignore the commandline flags used for modular analysis in order to avoid breaking build pipelines that were passing them. We may remove these at a later date.
Change-Id: If574ce2358280ab5fedd89c62665328601e72e22
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/333360
Reviewed-by: Mayank Patke <fishythefish@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
The only constructor invocation of `NonConstantValue` is in the deserializer. Which means we're never creating any of these objects to serialize.
Also remove unused `handleClosureCall` method in `pkg/compiler/lib/src/inferrer/builder.dart`
Change-Id: I04e492bc4824795dd8fc109ee7464be56b27cb93
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332500
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
1. Add `Object` and `Map` to the very short list of names that are
unminified in the app.
2. Tweak test to avoid testing minified name.
Change-Id: Ide0cedc2950b5392eb6963403a48c0f89cd1b50a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332368
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
- Remove unused Map in source map emitter logic.
- Buffer output writing for both JS and source maps. Previously we were only buffering the source map writes.
During local compilations of a large program combined these amount to a >100MB improvement in memory usage.
Change-Id: I633c2f81aa28744e30c6a706bb3927423b38a6e0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331361
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
After further review the slight increase in memory usage is minimal and worth the improvement in runtime for each phase.
The new indices follow the same format as before, we assign each entity an index when it is registered into the entity map. However these new indices are only used by the entity maps and not as part of the serialization format for those entities. The index only exists to make look ups faster in the "map". As such, I've made the field private to prevent/discourage outside use of it.
Change-Id: I7a7c20a2dd51b01d2390bb4401545821a1014832
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331360
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
We recently added the ability to pass a `--write-dump-info-data=` flag to Dart2JS's emitter phase which will serialize the necessary metadata to run dump info as a standalone phase of the compiler.
This extra metadata was incurring an overhead since we were maintaining JS AST nodes longer in order to extract metadata out of them. With this change we get rid of that overhead. The nodes are only retained until they printed and then any references to them are freed.
After this change the memory usage of the emitter phase is the same with or without the `--write-dump-info-data=` flag. There is still however some additional overhead in runtime. This is of course unavoidable as we are doing extra work to serialize the data.
Change-Id: Ib45a0e1de96d41503cbd76217b841ff013ac0655
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331000
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
Given the change to use serialization indexing, these classes aren't necessary anymore. We can use the base entity and J- impls directly.
Change-Id: I1074429ca7165fa1dbf740cab96a1b432cdd0a3d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/326500
Reviewed-by: Sigmund Cherem <sigmund@google.com>
The element map indexing system adds complexity to our element model with little benefit. Serialization already has its own indexing logic that we now reuse for entities.
Some things to note:
1) Libraries are still assigned an index. We only use that index in one place in the namer to disambiguate members. I now assign these indices as a late final field when adding the libraries to the element map.
2) The sizes of serialized files get slightly bigger since serialization indexing uses "addresses" rather than incrementing numbers for the indices. This effect is very minimal (<4MB+ for the largest programs).
3) "Late" members (constructor and generator bodies) which are created during codegen must be tracked and registered separately. Before the index was used to determine which entities were "late".
I see a very small memory improvement across all phases because of this change. There is also a small regression in runtime because the entity map queries are hash map lookups instead of list index lookups now. I think this very small regression is worth the reduced complexity in the compiler.
If we care about getting that time regression back we can add an "IndexedEntityMixin" with a late final index member that the registry would set/manage internally and would allow it to go back to tracking entities in a List. The field should be private so it would only be used for this purpose. This would only be necessary if we find the added time overhead of the map lookups to be too much.
Change-Id: Idd6d22bfc6ab61943a07feb58b35287d50e7d72e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/326461
Reviewed-by: Mayank Patke <fishythefish@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Stephen Adams <sra@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
Simplifies indices management for serialization by reusing the same indexing readers/writers for each phase. This way the caches are shared across all the phases and we can decouple the indices with any specific data read.
This fixes an issue that prevented the indices from being used in certain read/write patterns. None of our current usages of these indices exhibited this pattern but later changes I have planned do run into this issue.
This new pattern is also more resilient to sharding/parallel data files. Previously there could be collisions if two files were written in "parallel" (such as the codegen shards) and later their address spaces had to be merged. Now we do the address space merging at read time when we have full knowledge of the parallel files and can therefore avoid collisions.
Change-Id: Iff4c1461e734fc00f251d81f9fff1b9db83484d6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/326460
Reviewed-by: Stephen Adams <sra@google.com>
Unfortuantely this golden test is set up to test two configurations, the
default dump-info and a experimental kernel-first dump-info. The latter
wants to reuse the same expectations to ensure the output is the same,
but it's configuration confuses how `dump_info_test.dart -g` works. If
it sees a discrepancy it will duplicate the expectation and add a new
one instead of updating the old one.
I haven't investigated the best way to fix that, but for now, this CL
remerges the expectation so that we continue to only have one copy of
the expectations.
Change-Id: I5d2781b85ae92e8e83b6a4c395a7353bcbcca17a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331321
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Stephen Adams <sra@google.com>
Auto-Submit: Sigmund Cherem <sigmund@google.com>
This adds the representation field of an extension type declaration
as an abstract getter in the ExtensionTypeDeclaraiton marking it as
a ProcedureStubKind.RepresentationField
These are never used as interface targets and are therefore skipped
in the type flow analysis.
TEST=existing
Change-Id: Ie645e63e0995a31895e985a2025dccb1476d16bf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/330782
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Ömer Ağacan <omersa@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
This CL reorganizes our collection of kernel transformations into
"modular" and "global" ones. Modular (phase 0a) transformations are
those that can be run on each library before we have "linked" the full
program AST. Global (phase 0b) transformations are those that are run
over the full program AST.
Although we colloquially refer to multiple transformations of each kind,
there is actually a single modular (resp. global) `Transformer`, which
merges all the required transformations into a single AST pass.
Change-Id: I8f53cba6fc9a8aab106188ec3597ab194dd0cde0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/330170
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Nate Biggs <natebiggs@google.com>
Use URLSearchParams to escape query parameters.
- For large query parameters (>100KB), where escaping the parameters causes jank, this can be 2x-5x faster, reducing ~100ms pauses to nearer frame rate.
- For small query parameters (<100B) it can be slightly (10-20%) slower, but still well below 1 millisecond.
TEST=ci
Issue: #53712
Change-Id: I045bac7a067a658a58aaac4266409d526ccda774
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/329822
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
Reviewed-by: Ömer Ağacan <omersa@google.com>
Reviewed-by: Nicholas Shahan <nshahan@google.com>
Reviewed-by: Lasse Nielsen <lrn@google.com>
This adds a list of `Procedure`s to `ExtensionTypeDeclaration`. This is
meant to model representation fields and combined member signatures
computed from inherited non-extension type members.
These are not meant to be handled by the backends. The combined
member signature can be the interface target of an `InstanceInvocation`
expression but will always have a `.memberSignatureOrigin` value from
one of the original class members.
TEST=existing
Change-Id: I87768ed75a3c7126b0a30f8ccf06e46678c56db6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/330301
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Srujan Gaddam <srujzs@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>
Commit-Queue: Johnni Winther <johnniwinther@google.com>
This renames ExtensionType.typeErasure and adds it to DartType. This also fixes the extension type erasure for when extension types are used in the arguments of an extension type.
DartType.extensionTypeErasure can be used by backends to easily
access the type without extension types for any type.
TEST=pkg/kernel/test/extension_type_erasure_test.dart
Change-Id: Ia49d273ed85111e3ae822720860a3e0be5ea0252
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/329960
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Reviewed-by: Srujan Gaddam <srujzs@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Johnni Winther <johnniwinther@google.com>
Today dump info generates the entire JSON string in memory before writing it out to disk. The JSON encoder supports chunked conversion so we can do partial writes to disk and keep less of the output in memory. When converting chunks the encoder writes to the sink a lot so we also need a buffering layer to limit the number of writes.
For large programs the dump info output can be almost 2GB. After some testing a buffer size of 500MB seems to consistently reduce memory consumption by ~3.5GB.
Note: This stacks with the changes in https://dart-review.googlesource.com/c/sdk/+/322843 to reduce memory consumption by 4.8GB on a local build (18.3GB -> 13.5GB).
Change-Id: Id8f359af0724c3925f2ff767caee80d949c46da9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323040
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
- FrameEntry needs to be cached since equality of SourceLocation.inliningContext needs to be maintained across the serialization gap.
- Global names should be sorted so that they are registered in a consistent order and thus get consistent indices.
- We should only build constructor bodies that are reachable as determined by the codegen enqueuer.
- Each occurrence of BoolConstantValue should map to a unique LiteralBool object. If the same literal is used, the source location map uses the last registered location for every occurrence of that literal. `js('true')` and `js('false')` cache the interpolated values so we cannot use those.
Change-Id: I731aa3f6c284323342a8844f4735fbcea7e553ef
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327060
Commit-Queue: Nate Biggs <natebiggs@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Dart2JS uses the file offset on these nodes to create JS source maps. We want to maintain these file offsets across the serialization layer when we compile from dill files rather than directly from sources.
Dart2JS tests were serializing via `--test-mode` flag and thus had incorrect source locations for some stack traces.
TEST=Updating tests for all of these in fasta offsets tests.
Change-Id: I33862462fbff84d88f8c51bdeb1efc5771cfb8b2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327160
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
This CL introduces StructuralParameter and StructuralParameterType
classes. They are intended to replace TypeParameter and
TypeParameterType respectively where those were used as type
parameters defined by FunctionTypes. Previously, type parameters of
FunctionTypes were represented by TypeParameter objects with the
‘parent’ field set to null. By introducing StructuralParameter and
StructuralParameterType this CL unambiguously separates the two
notions of type parameters.
TEST=existing
Change-Id: Ida3feb7ad96a7b2acef55840eacba9e36bf2a3e7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/312264
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Commit-Queue: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Nate Biggs