sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Support multi-option contexts

Open pq opened this issue 2 years ago • 21 comments

The Dart Analyzer treats the presence of an analysis_options.yaml file as a signal to create an analysis context, a discrete, hermetic environment for analysis. In many cases this is unneeded and the memory overhead of elements that could be shared but are duplicated is significant, contributing to poor IDE performance.

Plan: Decouple options from contexts, allowing one context to contain many analysis options files without any element model and file state duplication. To the user, there will be no change in semantics with reduced memory pressure and improved performance.


Follow-ups:

  • [x] #56352

Blocking:

  • [x] https://github.com/dart-lang/sdk/issues/55580
  • [x] https://github.com/dart-lang/sdk/issues/55413
  • [x] #55300
  • [x] https://github.com/dart-lang/sdk/issues/55252
  • [x] #54858
  • [x] https://github.com/dart-lang/sdk/issues/54849
  • [x] https://github.com/dart-lang/sdk/issues/54816
  • [x] #54714
  • [x] #54814
  • [x] #54791
  • [x] #54770
  • [x] #54312
  • [x] https://github.com/dart-lang/sdk/issues/53873
    • [x] deprecate uses (and update callers)
    • [x] remove API
  • [x] https://github.com/dart-lang/sdk/issues/54376
  • [x] https://github.com/dart-lang/sdk/issues/54043
  • [x] https://github.com/dart-lang/sdk/issues/54074
  • [x] #54124
  • [x] #54310

Related:

  • [ ] https://github.com/dart-lang/sdk/issues/54667
  • [ ] https://github.com/dart-lang/sdk/issues/54045
  • [ ] https://github.com/dart-lang/sdk/issues/54000

pq avatar Oct 26 '23 19:10 pq

Status 02.02.2024

Doing some initial testing on a local build, I've confirmed that flipping the flag "works" and DAS appears to be working as intended.

For my local analyzer SDK development workspace, I'm seeing the expected reduction in contexts.

Multi-Option Contexts (future)

image

Single-Option Contexts (today)

image

It's interesting to note the less than dramatic difference in memory usage.

Likely this can be at least partly explained by the nature of the saved contexts (in all 3 cases they have very few libraries) but the fact that this is essentially a cold-start capture makes these numbers imprecise anyway. Much more interesting will be to measure memory footprint over time (and with more substantial nested contexts).

Testing

For the curious, to enable mutli-option contexts in an SDK, set singleOptionContexts to false

image

and do a build.

pq avatar Feb 02 '24 19:02 pq

I think the .analysisOption may have been deprecated a bit early.

If .getAnalysisOptionsForFile(file) is still experimental, we don't have a stable replacement for .analysisOptions

rrousselGit avatar Feb 19 '24 08:02 rrousselGit

Sorry for the confusion @rrousselGit. This is taking a bit longer than hoped (lots of moving parts).

Out of curiosity, how are you using analysis options objects? In plugins?

pq avatar Feb 20 '24 15:02 pq

In analyzer_plugins, yes. In custom_lint specifically, a wrapper around analyzer_plugin to make it more dev friendly and improve support for monorepo in terms of memory usage.

rrousselGit avatar Feb 21 '24 00:02 rrousselGit

Thanks! If it isn't too much trouble, could you tell me how you're using analysis options in custom_lint?

(fyi @srawlins.)

pq avatar Feb 21 '24 05:02 pq

Any updates on this effort, @pq , as a P1 feature?

srawlins avatar Feb 28 '24 17:02 srawlins

Thanks for the ping. This is currently blocked behind #54858. Will be revisiting that in the next week.

pq avatar Feb 28 '24 18:02 pq

Investigating current blocked (https://github.com/dart-lang/sdk/issues/55252) now.

pq avatar Mar 20 '24 18:03 pq

Fix for #55252 cleared up the pkg bots 🎉. Waiting on flutter try results. 🤞

pq avatar Mar 22 '24 23:03 pq

I switched the singleOptionContexts flag to false, rebuilt the SDK, but I get same amount of context for our mono-repo project. Is this supposed to make any difference in monorepo projects?

knopp avatar Mar 25 '24 17:03 knopp

Is this supposed to make any difference in monorepo projects?

@keertip

pq avatar Mar 25 '24 17:03 pq

The singleOptionContexts flag will only make contexts due to nested analysis options files unnecessary so it's sort of orthogonal to monorepos.

For example, with singleOptionContexts set to true the following will produce two contexts

my_package/
  analysis_options.yaml
  lib/
  test/
    analysis_options.yaml

With it set to false, you should see only one.

If you don't have any nested options files, you won't see a difference.

pq avatar Mar 25 '24 21:03 pq

On the other hand, it's a necessary part of providing good monorepo support. Without this, every package in a monorepo would likely still have it's own analysis context, which would largely defeat the gains we expect to see from the monorepo support.

bwilkerson avatar Mar 25 '24 21:03 bwilkerson

Thank you for the confirmation. I'll patiently wait for pub workspace support.

knopp avatar Mar 25 '24 22:03 knopp

Any updates?

bwilkerson avatar Apr 15 '24 17:04 bwilkerson

We're waiting for https://dart-review.googlesource.com/c/sdk/+/362442 to fully make it through a flutter roll before we can try to re-enable this. (See: https://github.com/dart-lang/sdk/issues/55413)

pq avatar Apr 15 '24 17:04 pq

https://github.com/dart-lang/sdk/commit/22da6ed1ccc84fa5fb31433ba4465542733195fe flips the bit but we've reverted related changes twice so I think we want to let it bake for a few more days at least before declaring victory.

pq avatar Apr 23 '24 18:04 pq

Looking at the latest engine roll (https://github.com/flutter/flutter/pull/147373) which brings Dart in @https://github.com/dart-lang/sdk/commit/b5f51d8868191b5358ccb11f07e77bf3fd3bd09b, it looks like this change has safely made it's way into Flutter. Here's hoping this landing sticks. 🤞

pq avatar Apr 25 '24 17:04 pq

With https://github.com/dart-lang/sdk/issues/55580 fixed, all known issues have been addressed.

pq avatar May 07 '24 17:05 pq

I think we can close this? Can we mark AnalysisContext.getAnalysisOptionsForFile non-experimental?

srawlins avatar Oct 16 '24 00:10 srawlins

I'm in favor but have a few reservations. I've added it for discussion later today.

pq avatar Oct 16 '24 14:10 pq

Performance Reflections

Looping back since we never quantified performance impact, here's a quick comparison of memory and runtime data grabbed from "before" (3.5.0-72.0) and after (3.5.0-109.0) SDKs using an SDK checked out at https://github.com/dart-lang/sdk/commit/22da6ed1ccc84fa5fb31433ba4465542733195fe for source analysis.

Memory Pressure

A crude measure of memory pressure is to compare server statistics for the sdk.code-workspace (which includes SDK packages) on a cold start where we see a 22% improvement.

Cold-Start Before (3.5.0-72.0)

image

Cold-Start After (3.5.0-109.0)

image

In practice the benefit is significantly higher since each context with open files will accumulate elements cached in memory so it's significant to notice that in the after case we have 53 contexts vs. 129 before (a reduction of almost 60%).

Wall Time

An approximation of runtime performance improvement gleaned from comparing wall times of analysis runs shows a roughly 45% improvement when analyzing the pkg directory in the SDK.

Analysis Before (3.5.0-72.0)

( repeat 5; do; ./dart-sdk-pre/bin/dart analyze <dart>/sdk/pkg; done; ) 39.19s user 12.63s system 128% cpu 40.269 total

Analysis After (3.5.0-109.0)

( repeat 5; do; ./dart-sdk-post/bin/dart analyze <dart>/sdk/pkg; done; ) 24.36s user 8.21s system 128% cpu 25.395 total


While imprecise, these numbers are useful and support what users have been reporting and anecdotally we've been experiencing since turning the feature on. (Note also that workspaces makes life better still in a monorepo configuration. 🎉 )

pq avatar Nov 20 '24 18:11 pq

Closing this feature as done. Any follow-ups can be tracked in their own issues.

pq avatar Nov 20 '24 19:11 pq