Feat: Add unused dependency cleanup

[rkishan516]

Feat: Add unused dependency cleanup fixes: #106549

Before this PR

If any widget add dependency to other InheritedWidget, it never gets removed until widget is out of tree. e.g.

class ButtonDependsOnTheme extends StatelessWidget {
  const ButtonDependsOnTheme({super.key});

  @override
  Widget build(BuildContext context) {
    return ElevatedButton(
      onPressed: () => Theme.of(context),
      child: const Text('Tap'),
    );
  }
}

Once the button is tapped, any change in theme is applied to ButtonDependsOnTheme widget.

After this PR

If any widget add dependency to other InheritedWidget and in next rebuild it doesn't keep it as in example, it will be removed from dependency.

Pre-launch Checklist

• [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• [x] I signed the [CLA].
• [x] I listed at least one issue that this PR fixes in the description above.
• [ ] I updated/added relevant documentation (doc comments with ///).
• [ ] I added new tests to check the change I am making, or this PR is [test-exempt].
• [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
• [ ] All existing and new tests are passing.

[Piinks]

Hi @rkishan516 do you still have plans for this change?

[rkishan516]

Hi @Piinks, This was a proposal PR for @justinmc to review. If we want to continue with this, then I can surely update this.

[Piinks]

This has come back around to stale PR triage. We don't review draft PRs, but have you reached out on Discord for the feedback you were seeking?

[rkishan516]

This has come back around to stale PR triage. We don't review draft PRs, but have you reached out on Discord for the feedback you were seeking?

Hey, last time we couldn't conclude, I will continue that discussion again today.

[benthillerkus]

It looks like there are two failing customer tests in the provider package, and they're kind of concerning. I wouldn't expect this change to cause them to fail, but maybe it's because I don't understand the inner workings of provider. Do you know what's causing the failure?

https://github.com/rrousselGit/provider/blob/558bdcd26ddb2b922bec06e3fa0d91bc59479baf/packages/provider/test/null_safe/context_test.dart#L489-L520

The test is incorrect as per Providers own documentation: https://pub.dev/documentation/provider/latest/provider/ChangeNotifierProvider-class.html#:~:text=DON%27T%20reuse%20an%20existing%20ChangeNotifier%20using%20the%20default%20constructor

I think I agree with @rrousselGit in the original issue (#106549), who said that this should probably be opt-in. I really can't think of any reasonable situation where you would logically need the original behavior, can you? But I worry due to the (slight) performance effect.

Overall I agree that Flutter should have this feature, probably behind an opt-in flag.

Maybe tests could be run if this actually affects performance? Because I can also come up with some pathological cases where this should boost performance.

IMO it would be too bad to have the desired / correct behaviour only available behind a flag / different API & having to wait for the entire ecosystem to migrate.

[rkishan516]

Thanks @justinmc! I've investigated the failing provider tests and found the bug in my implementation.

The issue was in where cleanupRemovedDependencies() was being called. My original implementation called it only once on the root context element in buildScope(), but _flushDirtyElements rebuilds many elements during a build cycle.

For the fix, I moved cleanupRemovedDependencies from BuildOwner.buildScope() to ComponentElement.performRebuild. Now each element cleans up its own dependencies after it rebuilds.

On the opt in choice, I am not really sure. I can see some advantage of opt in, so for now I am also leaning towards it.

[dkwingsmt]

Since the current behavior (never unsubscribe) is intentional, I assume we'll never make this behavior the default. In this case, do you think it'll be easier to have another method to call than to configure this when defining the entire class? This allows the app developers to decide whether to adopt it, so that this feature can be applied to existing inherited models as well.

I'm imagining a removeDependency method, since the developer should know the timing that they're no longer using the dependency. Or we can have a slightly smarter method such as checkDependencyCleanup. In general my idea is that if the developer feels like unsubscribing, they can call it in similar places as the dependOn methods.

[rrousselGit]

The very reason this feature is requested is that developers dont have a good place to unsubscribe.

Folks can already override InheritedElement.removeDependent. They thing is that the only mean for a package to know when a dependency is no-longer useful is to override ComponentElement.build. That requires dissociating from Stateless/StatefulWidget, making this solution incompatible with a large chunk of the ecocystem.

(the alternative of providing Builder-like widgets has the same compatibility issue)

[rkishan516]

@dkwingsmt I don't think without breaking change we can add any new perameter or method. Also as @rrousselGit mentioned developers dont have a good place to unsubscribe.

I went with current approach, because

• InheritedWidget can override cleanupDependency to return true (if they want)
• The framework automatically removes dependencies when widgets are removed from the tree or stop calling dependOnInheritedWidgetOfExactType
• Its simplest way to keep API backward compatible.

[dkwingsmt]

I see. Thank you all for explaining, and sorry for the confusion.

[rkishan516]

I wonder if the performance optimization that Hixie mentioned was about these two kinds of calculation or the work to actually unsubscribe. But nevertheless, considering the impact of this change, I'd hope for a performance analysis, either through an existing benchmark or a new benchmark that builds with thousands of dependencies.

I have added few benchmark test.

[Piinks]

I have added few benchmark test.

Thanks for adding this!

And thanks @dkwingsmt, that was an excellent recommendation to add performance analysis here.

I think, in order to do this properly, we should land the benchmark separately, let it collect information and establish a baseline. Then we can actually see if this change has an impact in the benchmark that would suggest whether or not this change would negatively impact performance. Does that sounds right to you @dkwingsmt?

[dkwingsmt]

I agree with @Piinks . In terms of execution, I recommend the following process:

• Phase 1: Investigation. For now let's simplify the benchmark test in this PR to only include the no-cleanup case, since this is the only thing blocking this PR. Once that's done, perform the benchmark on main and on this PR respectively to compare the results. Once we're happy with the result and the implementation, move on to Phase 2.
• Phase 2: Split the benchmark test into a separate PR and land it. Wait for ~~a while~~ a few commits for the benchmark to be recorded, and move on to Phase 3.
• Phase 3: Land this PR. Optionally, you can now expand the benchmark test to include the with-cleanup case in this case for future regression watch.

[Piinks]

Thanks @dkwingsmt! I think to be clear we should have a least a couple of commits land between phase 2 and 3 to make sure we have enough information for a useful baseline. :)

[dkwingsmt]

I simplified the test to only include the case without cleanup and ran it on macOS, and here's the result:

main: 

flutter: Rebuild (1000 deps, stress test): 8413920.0 ns per iteration
Rebuild (2000 deps, stress test): 16662090.0 ns per iteration
Rebuild (5000 deps, stress test): 115331060.0 ns per iteration

With this PR: 

flutter: Rebuild (1000 deps, stress test): 8417710.0 ns per iteration
Rebuild (2000 deps, stress test): 16744100.0 ns per iteration
Rebuild (5000 deps, stress test): 112770620.0 ns per iteration

(I don't have a low-end device to test. I tried to run it on my iPhone Air and won't give significant result without a depth > 2000, which then causes stack overflow. It would be great if someone can run it on a lower-end device after we finishing the tests.)

It seems to me that the time difference is at most < 0.4% (depth 2000) and is likely flakiness, since the case with depth 5000 even runs faster with this PR.

I still have one more concern, since this test only checks the case with only 1 dependency kind, it might not truthfully reflect the behavior of real apps. For example, the set.difference is probably not doing much work, and the Dart VM might be using some kind of "small object optimization" for the small set. @rkishan516 Can you see if there's a way to dynamically generate a lot of dependency classes, so that the test not only expands in terms of depth, but also width? Thank you.

[rkishan516]

I still have one more concern, since this test only checks the case with only 1 dependency kind, it might not truthfully reflect the behavior of real apps. For example, the set.difference is probably not doing much work, and the Dart VM might be using some kind of "small object optimization" for the small set. @rkishan516 Can you see if there's a way to dynamically generate a lot of dependency classes, so that the test not only expands in terms of depth, but also width? Thank you.

I don't think we can add inherited widgets dynamically. But we can create 20 different types and use them to test width. WDYT?

[dkwingsmt]

@rkishan516 Yeah, let's see how it goes. Thank you!

[chunhtai]

The implementation most looks good to me, just left some suggestion to see if we can squeeze out as much performance as possible