There are no functional changes, just changes to the way the logging
functionality is implemented. The motivation for the changes is to
allow future CLs to explore ways of changing the expectations so that
async handling of messages won't produce flaky tests.
All references to the 'messageLog' outside the class have been replaced
by higher-level methods with a semantic meaning.
The class has been split into an interface and an implementation, which
allows the test-specific aspects to be in the `test` directory.
Some additional code cleanup was also done.
Change-Id: Ie515f71153a96fb6e92eb8e2eb05f4b5e064bbd7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421965
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
The choice to use a queue was somewhat arbitrary; I suspect that we
could just as easily use a list or a set. We might want to change it in
the future.
The important part of this CL is that we need to keep a reference to all
of the messages that are being executed so that we can cancel them. I'm
fairly sure that there is currently a bug that will prevent messages
from being cancelled if a later message has started running concurrently
because we only remember the most recently started message.
The next step is to update the test framework to allow for having the
order in which operations complete be different from run to run, and
then we can write tests to prove the fundamental properties that we
want the scheduler to exhibit.
Change-Id: If23fe7a61a8de24773b892157bbe301844b456ec
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421582
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
If we need to control when requests that modify the state of analysis
are performed, and we do, then that needs to include watch events from
a file watcher. This CL causes those events to be placed on the message
scheduler's queue so that we can control when we notify the analysis
driver of changes.
Change-Id: I1b099a35da7a4b9a6f4d86feb497d6822041155d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421520
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
When the property experiment is enabled, we may evaluate getters in inline values. For some types, the chance of these having side effects are much higher (for example accessing `length`, `last`, `first` on `Iterable`s or `Stream`s.
This suppresses inline values on these types (both the variables themselves to avoid any `toString()`s, and any getters).
Fixes https://github.com/dart-lang/sdk/issues/60402
Change-Id: Ie9b524a679df5e39856ecd900d94f2fb41b779bf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420703
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Sometimes - on responding to `edit.getFixes` requests, the method
`getTopLevelDeclarations` is called several times with the same
parameter and caching has then sometimes sped things up.
I then also noticed that it caused `reportAnalysisAnalytics` to be
called many times (e.g. 73 times for one `edit.getFixes` request)
each time spending time collecting data which - on passing to
`analyticsManager.analysisComplete` - was just thrown away because it
only saves the first one.
Combined these changes have been observed to reduce the response time
on `edit.getFixes` by up (down?) to ~20% (e.g. from 5 seconds
to 1 second).
In response to http://b/407797012 where more data is also available.
Unfortunately I haven't been able to reproduce this is a benchmark
(yet anyway). I don't know if it needs slower slower reads, an extreme
amount of files, several contexts or something else entirely.
Change-Id: Id214b0e1b7d8bbef7f6d956408f17fab45618b78
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420323
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
This CL makes three changes:
- moves the message scheduler into a new directory,
- splits the implementation into two files,
- and renames `MessageObject` to `ScheduledMessage`.
It does not make any semantic changes to the code.
The expectation is that we will need to make the message scheduler more
complex as part of improving the performance of the server, and that it
will be easier to do this if the code is structured differently.
Change-Id: I3f8741cadceb34c69fd45c1bf21f331d15824565
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420500
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
A few statements around the package read:
> Producers used in bulk fixes must not modify the FixKind during
> computation.
In fact, this `add_null_check` correction producer seems to be the
_only_ remaining producer that violates this statement. In order to
fix it, such that `fixKind` is set during constructor initialization,
we must do some poking around the AST.
So we convert `_computeTarget` to be static (and change it to also
compute and return any possible null-aware token, renaming it to
`_computeTargetAndNullAwareToken`). We also convert `_isNullAware` to
be static. Then the two public constructors are converted into
factory constructors that determine the fix kind, and store the
`_target` and `_nullAwareToken` in fields, so as to avoid re-computing
them during `compute()`.
Change-Id: Ic66319ef764dd9bd69f0e1059347acf174debb25
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419420
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
The `DartFixContext.librariesWithExtensions` utility function accepts
a `memberName` parameter but never did anything with it. This change
adds filtering in that function based on the member name.
This code depends on the set of exported extensions, which is provided
in an extension (`LibraryElementExtensions.exportedExtensions`) in the
analysis_server package, which must be moved to the analyzer package.
Change-Id: I06c8a9a0e11ceb70e75517b30c0b048321d6b024
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418641
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@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>
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>
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>
The motivation here was to remove the local duplicate variables,
`isEmptyIdentifier_final`, `context_final`, and `returnType_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.
This CL simplifies each method by taking advantage of early returns
and implicit late assignment.
Change-Id: I81ff248d97e875a5b75c7d20b8e95f1b0c08ea24
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417701
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
The motivation here was to remove the local duplicate variable,
`final_propertiesBuilderName`. This variable only existed because the
variable it duplicates loses its promoted type inside the `builder.addDartFileEdit` closure. It loses its promoted type because
it is multiply assigned in a for loop.
This CL extracts a portion of the `compute()` method, which is solely
concerned with computing the real "target". The diff looks large
because of how that portion is moved into a helper method that is
_below_ `compute()`.
I think each of these changes then improves readability: extracting
part of a 180-line long method into one helper, and avoiding a local
duplicate variable.
Change-Id: I96b6ccdbd627d3cf3cc4b7f7bbcba7ee5dd9a561
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417700
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
For convenience, we usually include Inline Values for the "current" line so that when stopped at an `if` statement, you can see their values:
```
if (foo == 1) { // [foo = 2]
```
We have an experiment that includes inline values for getters (since although they can have side-effects, users tend to expect to see them in debug views, especially when the getter/setter are implicit), but triggering them before the code ever would have may be more unexpected.
This change uses two different ranges so that only variables are included for the current line.
Change-Id: I19a129a28f7b5296281dbee139855c738cd37194
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418340
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
The motivation here was to remove the local duplicate variable,
`node_final`. This variable only existed because the variable it
duplicates loses its promoted type inside the `builder.addDartFileEdit`
closure. It lose its promoted type because they are multiply assigned
in a while loop.
This CL extracts the first portion of the `compute()` method, which
is solely concerned with computing the real "target". The diff looks
large because of how that portion is moved into a helper method that
is _below_ the second half of `compute()`.
I think each of these changes then improves readability: extracting
part of a 140-line long method into one helper, and avoiding a local
duplicate variable.
Change-Id: Ic209e4be49863657f969eadf909ab05d13e9a254
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417680
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
To support this feature:
* Remove `DartAssistContextImpl` as it has no differences from
`DartAssistContext`. Thus the getters `DartAssistContext` are made
into fields, and all callers now call `DartAssistContext()`.
* Add a top level `computeAssists` function, similar to the top-level
`computeFixes` function.
Change-Id: I3ef5fd9ee9430150004b29c11ab972939b85b214
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417360
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
This CL adds the option of sending arguments to the benchmark(s)
* Specify what dart to use (e.g. `--dart=/path/to/dart-sdk/bin/dart`),
e.g. if wanting to run benchmarks on an (old) released version.
* Specify the number of files to operate on (e.g. `--files=100,200`).
* Specify the code-types to benchmark
(e.g. `--types=ImportChain,ImportExportChain`)
* Specify verbosity, e.g. to debug (maybe `--verbosity=4`) or to run
benchmarks several times only to look at the final result
(`--verbosity=-1`).
Change-Id: I5c731f3e6480afdff0546a29c599430d2ab6e7be
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417460
Commit-Queue: Jens Johansen <jensj@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Currently, the `AnalysisContextCollectionImpl` constructor has a
callback parameter, `updateAnalysisOptions2`, which has 3 required
parameters. The caller can then pass in a closure which accepts those
parameters, and updates analysis options.
I find this to be a bit of an awkward API, and a little bit complex, so
it would be good if we can simplify it. It is all private impl, but it
is used in a few codebases, I believe. Dartdoc is one.
So, it looked to me like none of the callers _use_ the `ContextRoot`
parameter, so it would be good to simplify here, and remove it from
the callback signature.
This is _not_ the final API I'm looking for. Just an incremental step,
and since it is private API, it doesn't need to be in a major release
of analyzer.
Change-Id: Iad9da9f36e817add0bd8c49975ecd14311588661
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/392204
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Brian Wilkerson