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

[CP] [analyzer] Limit the number of plugins-per-context to one #51062

Closed
srawlins opened this issue Jan 18, 2023 · 14 comments
Closed

[CP] [analyzer] Limit the number of plugins-per-context to one #51062

srawlins opened this issue Jan 18, 2023 · 14 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@srawlins
Copy link
Member

Commit(s) to merge

0ab9f97 and 5fe2f3b

Target

beta

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/279324

Issue Description

Some users complain on social media that the Dart Analysis Server is slow and consumes a lot of memory. One theory is that these users may be using multiple analyzer plugins and are unaware that they are, or unaware that these multiple analyzer plugins are the culprit. To mitigate this issue, we are limiting the number of analyzer plugins per analysis context to 1.

What is the fix

The fix is to limit the number of analyzer plugins per analysis context to 1.

Why cherry-pick

It may be a pressing issue that some users have a poor editing experience due to multiple plugins being run simultaneously. We'd like to fix this before Dart 3.

Risk

medium

Issue link(s)

#50981

Extra Info

I write 'medium' risk, without really knowing what 'low', 'medium', or 'high' mean, for a few reasons:

  • This cherry pick required two CLs because the first had issues with the tests
  • This cherry pick had merge conflicts that I had to resolve
  • I am very very unfamiliar with analyzer plugins. I manually tested the fix, but not with extensive testing. No one in our organization uses analyzer plugins, much less multiple, so this isn't being dogfooded.
@srawlins srawlins added the cherry-pick-review Issue that need cherry pick triage to approve label Jan 18, 2023
@srawlins srawlins changed the title [CP] <title> [analyzer] Limit the number of plugins-per-context to one Jan 18, 2023
@srawlins
Copy link
Member Author

CC @jacob314 @mit-mit @devoncarew

@a-siva a-siva added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jan 19, 2023
@itsjustkevin
Copy link
Contributor

@srawlins based on your risk assessment, I don't know if I feel like this cherry-pick is worth the risk so close to the launch. Do we feel like this is impacting users enough to take the risk? Are we against having this in the first batch of hotfixes to stable post dart 2.19?

@srawlins srawlins changed the title [analyzer] Limit the number of plugins-per-context to one [CP] [analyzer] Limit the number of plugins-per-context to one Jan 19, 2023
@srawlins
Copy link
Member Author

@srawlins based on your risk assessment, I don't know if I feel like this cherry-pick is worth the risk so close to the launch.

I agree, but I'll let @mit-mit @bwilkerson and @devoncarew weigh in.

Do we feel like this is impacting users enough to take the risk?

We don't have much data at all, as we don't have analytics.

Are we against having this in the first batch of hotfixes to stable post dart 2.19?

I would be fine with that.

@devoncarew
Copy link
Member

For transparency, I haven't looked through the commits to eval them, but just based on the size, I would hold off on cherry-picking into beta for the launch.

But this is something we want in 2.19 generally; I would be in favor of cherry-picking it for the hot-fixes post-launch. It's an important issue to address and having this fix in 2.19 will help us with messaging to the community ('here are some upcoming changes...').

@devoncarew
Copy link
Member

(also, @srawlins, thanks for taking this on!)

@itsjustkevin
Copy link
Contributor

Thanks for weighing in @srawlins and @devoncarew with that, can we cancel the cherry-pick to beta and stage this for a cherry-pick to stable once we launch the next version?

@bwilkerson
Copy link
Member

sgtm

1 similar comment
@jacob314
Copy link
Member

sgtm

@pq pq added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Jan 23, 2023
@athomas
Copy link
Member

athomas commented Jan 30, 2023

Please re-evaluate for stable.

@itsjustkevin
Copy link
Contributor

@srawlins did we ever post a stable cherry-pick for this issue?

@whesse
Copy link
Contributor

whesse commented Feb 6, 2023

This has not landed on stable yet, and a stable hotfix will be built tomorrow.
Please create a CL targeting stable with this hotfix, and get it approved and landed.
This issue should also get approval before landing the cherry-pick.

@srawlins
Copy link
Member Author

srawlins commented Feb 6, 2023

I'll try to get to this today, but it could take a while. I wouldn't wait for me.

@srawlins
Copy link
Member Author

srawlins commented Feb 6, 2023

Replacing with #51272 to merge to stable.

@satvikpendem
Copy link

I'm using a few linting packages, and I'm getting this warning about having multiple plugins, perhaps it's useful to some people to be able to have more than one plugin active, maybe behind an advanced flag in analysis_options.yaml? I made a related issue here: invertase/dart_custom_lint#78

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests