In a follow-up CL, I intend to rewrite `ResultComparator` in a way
that doesn't depend on its base class, `AstComparator` (so that
`AstComparator` can be deleted). This change prepares for the rewrite
by adding unit tests for `ResultComparator`.
I've discovered some bugs in `ResultComparator` in the process:
- The failure messages reported when one of the two nodes was `null`
were reversed. I've gone ahead and fixed this as part of this CL,
since the fix was easy and didn't affect any tests beyond the tests
that I've added.
- The failure message reported when a node had the wrong type
contained an unnecessary newline. I've gone ahead and fixed this as
part of this CL, since the fix was easy and didn't affect any tests
beyond the tests that I've added.
- When comparing identifiers, if the actual identifier is a synthetic
identifier, and the expected identifier is any identifier at all,
the identifiers are considered to match. This is clearly not the
intended behavior; the intended behavior is that the expected
identifier must be `_s_` to match a synthetic identifier. I've
documented this bug with a TODO, which I'll address in a follow-up
CL.
- When comparing comments, only the references inside the comment are
compared; the text of the comment is ignored. This seems
inconsistent with the other behaviors of `ResultComparator`, and I'm
pretty sure it's not necessary. I've documented this bug with a
TODO, which I'll address in a follow-up CL.
Change-Id: Iba0169ded812d646b71966c2be22174d787c5a21
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419424
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Because certain union cases weaken the type constraint (the kind), it
was unsafe to pass the full set of flags rather than just the powerset.
Change-Id: Ia4a65d539669acbe696bc237230eb84a4b7d8c99
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417951
Reviewed-by: Nate Biggs <natebiggs@google.com>
For troubleshooting, this allows the client to override this flag.
I'd originally planned to use ClientConfiguration for this and allow it to be changed on-the-fly, however this can result in a mix of requests and therefore I decided it best to only support setting once during startup (which is part of initialization, and therefore before general requests start being sent).
This will require some Dart-Code work to provide a value here (and without a value, it will always be the servers default).
See https://github.com/dart-lang/sdk/issues/60440
Change-Id: Ie9843543d6d491afb046f3d1106211b7db852605
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419541
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
These test cases were broken by
https://dart-review.googlesource.com/c/sdk/+/252566 and were marked
with `@failingTest`.
The reason for the breakage was that the aforementioned CL changed the
implementation of `AstComparator.visitSimpleFormalParameter` so that
it compared the parameter's names using `isEqualTokens` rather than
`isEqualNodes`. When this logic is used by `ParameterListTest`,
`isEqualTokens` and `isEqualNodes` are being overridden by methods in
the derived class `ResultComparator`, and one of the special behaviors
of `ResultComparator.isEqualNodes` is to allow the simple identifier
`_k_` to be substituted for a keyword token.
To fix the test, this special behavior needs to be moved from
`ResultComparator.isEqualNodes` to `ResultComparator.isEqualTokens`.
This paves the way for a follow-up CL in which I plan to rewrite
`ResultComparator` so that it doesn't depend on `AstComparator`; that
in turn will allow `AstComparator` to be rmoved.
Change-Id: I8434c7c694e1e5c613df62bf34ee69126fb0c2ec
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418900
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Paul Berry <paulberry@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Introduce a helper library to trim components based on what we believe
it is needed for modular bytecode compilation. The script is configured
to accept a set of entry points, so unreachable libraries can be removed
entirely. The contents of the retained libraries is trimmed to remove
method bodies, constructor bodies, and initializers, except for where
they may be needed.
In the near future, this should be expanded to:
* include proper unit testing in the CFE
* review whether additional trimming operations can be made
* consider an explicit representation of trimmed content, to help the
CFE recover when assumptions are not met (e.g. sentinel markers
to establish whether a value has been trimmed)
* CFE produces trimmed data directly if needed, without having to first
produce the full dill.
Tests that specifically stress that we don't over-trim include:
apply_mixin (requires preserving method bodies), const_body (requires
preserving initializers).
TEST=existing and new e2e dynamic module aot tests.
b/394936876
Change-Id: I26db8385bdfe1664b2aea234ec8bb896c7c21230
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418702
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
When generating the `br_table` for the state machines in `sync*` and
`async` functions, we currently generate a default state (required by
`br_table`) with an `unreachable` instruction.
Instead, remove the default state and make the last state in the
function the default state.
This saves a few instructions per `sync*` and `async` functions.
Issue: https://github.com/dart-lang/sdk/issues/60433
Change-Id: Ie89c3581304a97082628b150cc5604f9e4c2849e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419540
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Ömer Ağacan <omersa@google.com>
Only in `Function.apply` we have the problem of determining
the order of named parameters for calling the closure. In all
other cases, the compiler can ensure the caller and callee
agree on the order.
There's one place where we determine this order. That place
can also be used to swap out user-provided `new Symbol()`s that
were passed to `Function.apply` with the canonicalized
`const Symbol()` instances the compiler uses for the
closure functions.
Doing so allows us to use `identical()` for symbol comparisons
everywhere else in the system.
Change-Id: Ib9833e708ba197c8b9ef18ef4bf9e7a64bdf9a40
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419201
Reviewed-by: Ömer Ağacan <omersa@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
When a TFA eliminates a field it moves it's initializer to a
`LocalInitializer` of the constructor. After [0] the
`VariableDeclaration` of the local initializer will get the type of the
field.
Though that CL didn't ensure TFA also visits the type of the
`VariableDeclaration`. This can lead to a situation where the type
refers to a class that TFA has tree shaken, which causes dangling
references in the AST.
[0] https://dart-review.googlesource.com/c/sdk/+/415201
Issue https://github.com/flutter/flutter/issues/166124
Issue b/406692736
TEST=pkg/vm/testcases/transformations/type_flow/transformer/write_only_field
Change-Id: Ieb8f5c4daedd314c613617218da898059bb6941a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419542
Reviewed-by: Slava Egorov <vegorov@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
Before this change, Slot::Equals would hit the default UNREACHABLE()
case if used to compare two equal native slots. That's surprising and
also unavoidable if code comparing arbitrary slots wants to check Slot
equality, since there's no existing predicate to determine whether an
arbitrary slot is native.
TEST=ci
Cq-Include-Trybots: luci.dart.try:vm-aot-linux-debug-x64-try,vm-aot-mac-release-arm64-try,vm-aot-mac-product-arm64-try,vm-aot-linux-product-x64-try,vm-aot-dwarf-linux-product-x64-try,vm-linux-debug-x64-try,vm-mac-debug-arm64-try,vm-mac-release-arm64-try
Change-Id: Ia794a33fc607013bf35435b0427ceb02bdae4b0d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419560
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Tess Strickland <sstrickl@google.com>
The legacy_many_files_in_flutter_set_subscriptions benchmark shows how
"flutter.setSubscriptions" calls can make the analyzer slower to
respond.
What happens is this:
* The user opens a new file in the IDE.
* The IDE sends the `flutter.setSubscriptions` request which equates to
a call to `getResolvedUnit` for each file in the request. If this is,
say, 300 files it's 300 calls to `getResolvedUnit`.
* The IDE sends a `edit.getAssists` request for the newly opened file.
This request starts processing, reaches `getResolvedLibrary(file)`
which calls `getUnitElement` ultimately adding the path to
`_unitElementRequestedFiles` which in `performWork` is done _after_
`_requestedFiles`, meaning it has to do all the flutter requested
files first.
* The user might then request completion for instance, but because the
analyzer only processes one request at a time it has to wait for the
`edit.getAssists` request to finish first, which had to wait for the
files from the `flutter.setSubscriptions` request to process.
All in all it's a lot of waiting for the user.
This CL adds a `interactive` option to the `getResolvedUnit` call. It
defaults to true in which case files are still added to
`_requestedFiles` and processed the same. If it's false it will instead
be added to a newly introduced list instead and processed at a lower
priority. Subscription requests are changed to pass `false` to
`interactive`, avoiding the scenario above.
Comparing before this CL with this CL on the
"legacy_many_files_in_flutter_set_subscriptions" benchmark with 100
files / CodeType.ImportChain these are the statistics on the changes
based on 5 runs each:
```
Completion after open of new file: -81.6652% +/- 7.7564% (-3.70 +/- 0.35) (4.53 -> 0.83)
getAssists call: -96.6315% +/- 0.9307% (-3.61 +/- 0.03) (3.74 -> 0.13)
peak virtual memory size: -5.6786% +/- 3.2964% (-139.00 +/- 80.69) (2447.80 -> 2308.80)
total program size (virtual): -4.6387% +/- 3.8146% (-110.80 +/- 91.11) (2388.60 -> 2277.80)
```
Even when https://github.com/flutter/flutter-intellij/issues/7980 is
hopefully fixed I think it is a fair change to de-prioritize a
non-interactive request.
Change-Id: Icba2faebf12f9913cf24db7cb90fdc6f4c74164e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418020
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Jens Johansen <jensj@google.com>
TL;DR: Add benchmark that shows the Dart 3.7
(https://dart-review.googlesource.com/c/sdk/+/386581 specifically)
introduced a regression.
Details: We've observed that sometimes the plugin that users has
installed times out (takes > 500 ms to answer).
This benchmark simulates that and shows the worse handling of this
introduced in Dart 3.7.
Running 10 iterations each of lsp_with_plugin_that_times_out.dart with
params 10, ImportChain:
Comparing 3.5.4 with 3.6.2
```
Initial analysis: -7.1855% +/- 2.9168% (-0.09 +/- 0.04)
peak virtual memory size: 11.8795% +/- 1.6185% (313.00 +/- 42.64)
total program size (virtual): 12.4293% +/- 1.2818% (323.10 +/- 33.32)
peak resident set size ("high water mark"): -7.4081% +/- 0.9050% (-38.90 +/- 4.75)
size of memory portions (rss): -6.3154% +/- 1.3641% (-32.00 +/- 6.91)
```
I'll note that this is only 10 files and we probably shouldn't put too
much weight on the initial analysis time here. Equivalently memory
probably isn't super interesting.
Comparing 3.6.2 with 3.7.2
```
Initial analysis: -45.8697% +/- 2.7193% (-0.53 +/- 0.03)
Completion call 1: 652.8573% +/- 1.3303% (0.97 +/- 0.00)
codeAction call 1: -0.7637% +/- 0.5174% (-0.01 +/- 0.01)
Completion call 2: 807.2503% +/- 0.7116% (0.89 +/- 0.00)
codeAction call 2: 0.2624% +/- 0.0456% (0.00 +/- 0.00)
Completion call 3: 868.9361% +/- 0.3199% (0.90 +/- 0.00)
codeAction call 3: 0.1433% +/- 0.0287% (0.00 +/- 0.00)
peak virtual memory size: -7.1307% +/- 3.4799% (-210.20 +/- 102.58)
total program size (virtual): -8.1674% +/- 3.5673% (-238.70 +/- 104.26)
peak resident set size ("high water mark"): -1.2546% +/- 0.7151% (-6.10 +/- 3.48)
size of memory portions (rss): -7.6891% +/- 0.9209% (-36.50 +/- 4.37)
```
Again I'll note that this is only 10 files and we probably shouldn't put
too much weight on the initial analysis time here - although we've seen
this improve in other benchmarks too so I do believe we have an
improvement here.
And again memory probably isn't super interesting because of the few
files.
The codeAction calls are basically the same which makes sense: In all
cases it's dominated by the plugin that times out.
The codeAction call does two calls to the plugin and it takes around
1000 ms (a timeout of 500 ms on each plugin call).
Here we see the regression in 3.7 clearly though (cut-out from above):
```
Completion call 1: 652.8573% +/- 1.3303% (0.97 +/- 0.00)
Completion call 2: 807.2503% +/- 0.7116% (0.89 +/- 0.00)
Completion call 3: 868.9361% +/- 0.3199% (0.90 +/- 0.00)
```
Because of https://dart-review.googlesource.com/c/sdk/+/386581 we only
do one request at a time. Before we would interleave the requests,
something like this:
```
Request 1: codeAction:
|--|waiting for plugin|--|waiting for plugin|--|
Request 2: completion:
|..--|
```
(I'm trying to ascii-draw that when the codeAction is just awaiting the
plugin the completion request can be executed)
Now instead we're doing this:
```
Request 1: codeAction:
|--|waiting for plugin|--|waiting for plugin|--|
Request 2: completion:
|..............................................--|
```
(I'm trying to ascii-draw that when the codeAction has to finish before
the completion request can be executed)
I'll also note that while it's here shown trough a plugin that times out
the same will likely happen in other circumstances as well. E.g. the
completion resolves with a special call `resolveForCompletion` which in
`performWork` gets priority over other things - but when not
interleaving requests that has very little effect.
For good measure, comparing 3.7.2 with HEAD
```
Completion call 1: 0.6851% +/- 0.2074% (0.01 +/- 0.00)
codeAction call 1: 0.6962% +/- 0.1876% (0.01 +/- 0.00)
peak virtual memory size: -11.7293% +/- 4.7947% (-321.10 +/- 131.26)
total program size (virtual): -12.1763% +/- 4.9125% (-326.80 +/- 131.85)
peak resident set size ("high water mark"): -14.7469% +/- 0.6980% (-70.80 +/- 3.35)
size of memory portions (rss): -10.4518% +/- 0.8927% (-45.80 +/- 3.91)
```
Not a whole lot of change for the time-related things on this benchmark.
Running legacy_with_plugin_that_times_out.dart shows the same thing
(here there's only 1 plugin call instead of 2 though; here only looking
at completion calls):
```
Comparing 3.5.4 with 3.6.2
Completion call 1: -6.1695% +/- 3.6147% (-0.03 +/- 0.02)
Completion call 2: -14.4462% +/- 8.8828% (-0.02 +/- 0.01)
Completion call 3: -16.7707% +/- 7.5412% (-0.03 +/- 0.01)
Comparing 3.6.2 with 3.7.2
Completion call 1: 139.2436% +/- 3.8814% (0.70 +/- 0.02)
Completion call 2: 1128.6580% +/- 14.8207% (1.41 +/- 0.02)
Completion call 3: 589.1425% +/- 11.9186% (0.78 +/- 0.02)
Comparing 3.7.2 with HEAD
no change on completion calls.
```
For fun, lets try to comment out `await completer.future;` in
`pkg/analysis_server/lib/src/server/message_scheduler.dart` that was
introduced in https://dart-review.googlesource.com/c/sdk/+/386581
(again only looking at completion calls):
legacy_with_plugin_that_times_out.dart:
```
Comparing 3.7.2 with HEAD (with commented out await)
Completion call 1: -58.5720% +/- 1.9196% (-0.70 +/- 0.02)
Completion call 2: -82.8149% +/- 1.1050% (-1.27 +/- 0.02)
Completion call 3: -85.6439% +/- 1.5128% (-0.79 +/- 0.01)
```
If looking at the values instead of the percent it looks to almost undo
the change from 3.6.2 to 3.7.2, so let's for good measure compare 3.6.2
with that:
```
Comparing 3.6.2 with HEAD (with commented out await)
Completion call 2: 95.7654% +/- 11.4279% (0.12 +/- 0.01)
```
So one of the calls is slower, but just double, not 10x. I haven't
looked into why.
And for lsp_with_plugin_that_times_out.dart:
```
Comparing 3.7.2 with HEAD (with commented out await)
Completion call 1: -92.9578% +/- 0.5505% (-1.04 +/- 0.01)
Completion call 2: -99.6372% +/- 0.0534% (-1.00 +/- 0.00)
Completion call 3: -99.6370% +/- 0.0429% (-1.00 +/- 0.00)
```
The 1000 ms wait time is gone.
And again let's for good measure compare 3.6.2 with that:
```
Comparing 3.6.2 with HEAD (with commented out await)
Completion call 1: -45.4578% +/- 2.4320% (-0.07 +/- 0.00)
Completion call 2: -96.6303% +/- 0.2481% (-0.11 +/- 0.00)
Completion call 3: -96.2054% +/- 0.3875% (-0.10 +/- 0.00)
```
Here all the completion calls are faster. I haven't looked into why.
Change-Id: I7c312f77b51bb4df68eedeb9bc6e27c2b0175cbf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418263
Commit-Queue: Jens Johansen <jensj@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
This change adds pragmas which can be used in dart:* libraries to
specify classes and members which are used by the compiler
to implement certain language features. Compiler can insert
references to those classes and members when desugaring language
constructs, so dynamic module code can use these classes and members
implicitly even if they are private.
Previously, we required to specify these members in the
dynamic_interface.yaml before dynamic modules could use them.
However, this is cumbersome and error-prone.
Now, the following pragmas are recognized:
@pragma('dyn-module:language-impl:extendable')
@pragma('dyn-module:language-impl:can-be-overridden')
@pragma('dyn-module:language-impl:callable')
If dynamic interface yaml file is specified during compilation,
then classes/members annotated with these pragmas are treated as if
they are used in corresponding sections of dynamic_interface.yaml.
If dynamic interface yaml file is not specified, these pragmas
are ignored.
Bug: b/395992622
TEST=existing
CoreLibraryReviewExempt: no API changes, only adding pragmas.
Change-Id: I6606467fc5e5264a2565d7eb8e8baccc2eebea84
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418663
Reviewed-by: Slava Egorov <vegorov@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
For out-of-memory events vm reserves (and if used, then replenishes) small chunk of memory for handling those, so preallocation of oom and stacktrace is not needed.
Unwind error can be created once as part of vm launch, kept in vm isolate.
This should reduce overhead of starting new isolate.
TEST=ci
Change-Id: Ibe2c85f250a521a8e838cc5690984fe9fa287a98
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419261
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Alexander Aprelev <aam@google.com>
The new name clarifies that the guide is specifically about element
model migration, not migration in general.
Changing the name now allows us to make links to the guide without
having to worry about updating those links if we change the name
later.
Change-Id: I6860867246b41671c20ecc725fdb2a5f7b77b949
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419240
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
The motivation here was to remove the local duplicate variables like
`final_foo` or `foo_final`. These variables only existed because the
variables they duplicate lose their promoted types inside the
`builder.addDartFileEdit` closure. They lose their promoted type
because they are multiply assigned in loops or not always promoted to
be non-null.
Often the fix is to replace a `if (x == null) return` with
`} else { return; }`. I think this code more directly represents the
flow of code, rather than relying on the nullity of a variable, like
"is this variable still null? Oh then we must not have entered any
of the situations above, so we should return."
Change-Id: I9dd6686b7f4d9c6caf59c171179b129a5a957ba8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418160
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
* Privatize `DartEditBuilderImpl.dartFileEditBuilder`.
* Rename the `toAdd` parameter on `_addAll` to `items`.
* Remove the unused `required` argument on `_canWriteType`.
* Privatize `DartFileEditBuilderImpl.createEditsForImports`,
`.codeStyleOptions`, and `ensureShown`.
* Remove the unused `useShow` parameter on
`DartFileEditBuilderImpl.importElementLibrary`.
Change-Id: I473788d9d566a2982914c374fce2429909466381
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418703
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Danny Tuppeny