Skip to content

[perf] Fix incorrect parentDisposables #8107

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.

Sign up for GitHub

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

Jump to bottom
Open
pq opened this issue Apr 23, 2025 · 3 comments
Open

[perf] Fix incorrect parentDisposables #8107

pq opened this issue Apr 23, 2025 · 3 comments
Assignees
@pq
Labels
P2 performance General performance issues (not perf tool) platform-idea tech debt

Comments

@pq
Copy link
Contributor

pq commented Apr 23, 2025 •
edited
Loading

Using an Application or Project as a parent disposable in plugin code can lead to plugins not being unloaded correctly and with that memory leaks.

Image

See: https://plugins.jetbrains.com/docs/intellij/disposers.html?from=IncorrectParentDisposable#choosing-a-disposable-parent
@pq pq self-assigned this Apr 23, 2025
@pq pq added platform-idea performance General performance issues (not perf tool) labels Apr 23, 2025
@pq pq changed the title Fix incorrect parentDisposables [perf] Fix incorrect parentDisposables Apr 23, 2025
@pq pq mentioned this issue May 8, 2025
Merged
1 task
jwren pushed a commit that referenced this issue May 8, 2025
@pq
[CQ] remove dead EditorEventServiceBase (#8163)
2e8f9e8 
Remove a bit more legacy inspector detritus.

Discovered when migrating parent disposables in pursuit of
#8107.


---

- [x] I’ve reviewed the contributor guide and applied the relevant
portions to this PR.

<details>
  <summary>Contribution guidelines:</summary><br>

- See our [contributor
guide]([https://github.com/dart-lang/sdk/blob/main/CONTRIBUTING.md](https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview)
for general expectations for PRs.
- Larger or significant changes should be discussed in an issue before
creating a PR.
- Dart contributions to our repos should follow the [Dart style
guide](https://dart.dev/guides/language/effective-dart) and use `dart
format`.
- Java and Kotlin contributions should strive to follow Java and Kotlin
best practices
([discussion](#8098)).
</details>
@pq pq mentioned this issue May 8, 2025
Merged
1 task
jwren pushed a commit that referenced this issue May 8, 2025
@pq
[CQ] remove stale FlutterAppManager (#8165)
7bae52b 
This service is unused.

Discovered when migrating parent disposables in pursuit of
#8107.

---

- [x] I’ve reviewed the contributor guide and applied the relevant
portions to this PR.

<details>
  <summary>Contribution guidelines:</summary><br>

- See our [contributor
guide]([https://github.com/dart-lang/sdk/blob/main/CONTRIBUTING.md](https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview)
for general expectations for PRs.
- Larger or significant changes should be discussed in an issue before
creating a PR.
- Dart contributions to our repos should follow the [Dart style
guide](https://dart.dev/guides/language/effective-dart) and use `dart
format`.
- Java and Kotlin contributions should strive to follow Java and Kotlin
best practices
([discussion](#8098)).
</details>
@pq pq mentioned this issue May 9, 2025
Merged
1 task
@pq
Copy link
Contributor Author

pq commented May 9, 2025 •
edited
Loading

#8178 gets us nearly there but I'm not sure what to do about AndroidModuleLibraryManager which has a few registrations like this:

flutter-intellij/flutter-studio/src/io/flutter/android/AndroidModuleLibraryManager.java

Line 211 in 0c1007c

Disposer.register(flutterProject, androidProject);

@alexander-doroshko: I wonder if you can advise? Are these Disposer registrations even necessary? If so, how would you migrate them?

pq added a commit that referenced this issue May 9, 2025
@pq
[PERF] migrate Project parentDisposables to Services (#8178)
f99f8cc 
Using a `Project` as a parent disposable can lead to plugins not being
unloaded correctly leading to memory leaks. This follows the advice
given in the Jetbrains [disposer
docs](https://plugins.jetbrains.com/docs/intellij/disposers.html?from=IncorrectParentDisposable#choosing-a-disposable-parent)
and is (I think) consistent with the Dart plugin (@alexander-doroshko?).

This gets us *most* of the way to fixing
#8107 with an open
question about `AndroidModuleLibraryManager`. (Question posed there 👍 .)

---

- [x] I’ve reviewed the contributor guide and applied the relevant
portions to this PR.

<details>
  <summary>Contribution guidelines:</summary><br>

- See our [contributor
guide]([https://github.com/dart-lang/sdk/blob/main/CONTRIBUTING.md](https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview)
for general expectations for PRs.
- Larger or significant changes should be discussed in an issue before
creating a PR.
- Dart contributions to our repos should follow the [Dart style
guide](https://dart.dev/guides/language/effective-dart) and use `dart
format`.
- Java and Kotlin contributions should strive to follow Java and Kotlin
best practices
([discussion](#8098)).
</details>
@alexander-doroshko
Copy link
Contributor

@pq Sorry for the delay. I can't answer this specific question right now. This question may disappear as soon as we start working on tech debt and the plugin refactoring.

General answer is that manual Disposer.register is very rarely needed. We try not to use the concept of Disposer/Disposables in new code at all.

So far, I don't fully understand the purpose of the AndroidModuleLibraryManager class. Extending and instantiating ProjectImpl (EmbeddedAndroidProject) looks suspicious by itself.

@pq
Copy link
Contributor Author

pq commented May 15, 2025

General answer is that manual Disposer.register is very rarely needed. We try not to use the concept of Disposer/Disposables in new code at all.

I wondered about this and kind of gathered as much.

So far, I don't fully understand the purpose of the AndroidModuleLibraryManager class. Extending and instantiating ProjectImpl (EmbeddedAndroidProject) looks suspicious by itself.

Right. There are lots of internal dependencies in this class and TODOs going that suggest it may not be working as expected. Some dating back to 2018!

It looks like I was a reviewer on this so I'll try and jog my memory. For reference there's #2474

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pq pq

Labels
P2 performance General performance issues (not perf tool) platform-idea tech debt
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pq @alexander-doroshko