Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multi-option contexts #53876

Open
22 of 24 tasks
Tracked by #53874
pq opened this issue Oct 26, 2023 · 21 comments
Open
22 of 24 tasks
Tracked by #53874

Support multi-option contexts #53876

pq opened this issue Oct 26, 2023 · 21 comments
Assignees
Labels
analyzer-analysis-options area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-performance Issue relates to performance or code size

Comments

@pq
Copy link
Member

pq commented Oct 26, 2023

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:

Blocking:

Related:

@pq pq changed the title no longer create contexts for analysis options files. stop creating analysis contexts for nested analysis options files Oct 26, 2023
@pq pq self-assigned this Oct 26, 2023
@pq pq added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-analysis-options type-performance Issue relates to performance or code size P1 A high priority bug; for example, a single project is unusable or has many test failures labels Oct 26, 2023
@pq pq changed the title stop creating analysis contexts for nested analysis options files Decouple analysis contexts from analysis options files Oct 26, 2023
copybara-service bot pushed a commit that referenced this issue Nov 28, 2023
This is a little hokey but I'm not sure it's worth over-thinking. Needless to say, input welcome!

Context: #53876

Change-Id: I9c249156ee2573dc3a74c38f939a0d4ba36609a6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/338583
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
copybara-service bot pushed a commit that referenced this issue Nov 29, 2023
…a contextRoot

I'm not crazy about the re-constitution of `File`s (see the TODO) but I'm not sure this is a yak we want to shave now (especially since this handler is not yet in use).

Larger context: #53876

Change-Id: I4b9814842525ed4440aa39a7d14baa75f526f72a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/339043
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
@pq
Copy link
Member Author

pq commented Feb 2, 2024

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 pq changed the title Decouple analysis contexts from analysis options files Support multi-option contexts Feb 2, 2024
@rrousselGit
Copy link

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

@pq
Copy link
Member Author

pq commented Feb 20, 2024

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?

@rrousselGit
Copy link

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.

@pq
Copy link
Member Author

pq commented Feb 21, 2024

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

(fyi @srawlins.)

@srawlins
Copy link
Member

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

@pq
Copy link
Member Author

pq commented Feb 28, 2024

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

@pq
Copy link
Member Author

pq commented Mar 20, 2024

Investigating current blocked (#55252) now.

@pq
Copy link
Member Author

pq commented Mar 22, 2024

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

@knopp
Copy link
Contributor

knopp commented Mar 25, 2024

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?

@pq
Copy link
Member Author

pq commented Mar 25, 2024

Is this supposed to make any difference in monorepo projects?

@keertip

@pq
Copy link
Member Author

pq commented Mar 25, 2024

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.

@bwilkerson
Copy link
Member

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.

@knopp
Copy link
Contributor

knopp commented Mar 25, 2024

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

copybara-service bot pushed a commit that referenced this issue Apr 8, 2024
#53876

Change-Id: Ia6e8fd299685f6049862a7027e9c7734d806e328
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/350342
Commit-Queue: Phil Quitslund <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
copybara-service bot pushed a commit that referenced this issue Apr 9, 2024
This reverts commit 409fa2c.

Reason for revert: breaking Flutter Engine -> Framework roll.

See: flutter/flutter#146506


Original change's description:
> enable multi-option context support
>
> #53876
>
> Change-Id: Ia6e8fd299685f6049862a7027e9c7734d806e328
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/350342
> Commit-Queue: Phil Quitslund <[email protected]>
> Reviewed-by: Brian Wilkerson <[email protected]>

Change-Id: I226d2d70fc15b83c26de015a96a2f328d6d03122
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362000
Reviewed-by: Alexander Aprelev <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
@bwilkerson
Copy link
Member

Any updates?

@pq
Copy link
Member Author

pq commented Apr 15, 2024

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: #55413)

copybara-service bot pushed a commit that referenced this issue Apr 22, 2024
See: #53876

With 0199e9d landed addressing #55413, this *should* now be safe to land.



Change-Id: I2c2a8d47ae0ede04f5de56b558f5e61778bf05f3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/364020
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
@pq
Copy link
Member Author

pq commented Apr 23, 2024

22da6ed 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
Copy link
Member Author

pq commented Apr 25, 2024

Looking at the latest engine roll (flutter/flutter#147373) which brings Dart in @b5f51d8, it looks like this change has safely made it's way into Flutter. Here's hoping this landing sticks. 🤞

@pq
Copy link
Member Author

pq commented May 7, 2024

With #55580 fixed, all known issues have been addressed.

@pq pq added P2 A bug or feature request we're likely to work on and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels May 7, 2024
@srawlins
Copy link
Member

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

@pq
Copy link
Member Author

pq commented Oct 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-analysis-options area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-performance Issue relates to performance or code size
Projects
None yet
Development

No branches or pull requests

5 participants