Skip to content

Invalidate caches after a hot reload #2641

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

Merged
merged 13 commits into from
Jul 4, 2025
Merged

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented Jun 28, 2025

Many of the caches we computed on the original load need to be invalidated. Previously, we pessimistically tore down the caches and reinitialized them. With this PR, a ModifiedModuleReport is computed that allows us to only invalidate and recompute information for modules/libraries that were either deleted or reloaded.

Specifically, the caches in MetadataProvider, Modules, Locations, SkipLists, AppInspector, and LibraryHelper are now specially invalidated and recomputed.

Removes an existing option to use the module URI instead of the module name in the MetadataProvider. This option is not relevant until we can unify this change across the ecosystem.

Fixes #2628.

@srujzs srujzs changed the title Invalidate deleted modules and libraries and process reloaded modules and libraries in provider Invalidate caches after a hot reload Jul 1, 2025
@srujzs srujzs requested review from nshahan and bkonyi July 1, 2025 03:43
Copy link
Collaborator

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick initial review.

In general, I don't know how I feel about overloading initialize(...) methods to also perform reinitialization. It'd be one thing if invoking initialize(...) simply reset the state to its initial state, but in many cases we're doing something more complicated than that. It'd be much clearer to have reinitialize(...) methods that are meant to be called after the first call to initialize(...) (it would be even better to only allow initialize(...) to be called once).

@srujzs
Copy link
Contributor Author

srujzs commented Jul 3, 2025

Thank you both for the review! FYI - I'm going to publish this once it lands so we can incorporate it into Flutter tools for the next Flutter stable release.

In general, I don't know how I feel about overloading initialize(...) methods to also perform reinitialization. It'd be one thing if invoking initialize(...) simply reset the state to its initial state, but in many cases we're doing something more complicated than that. It'd be much clearer to have reinitialize(...) methods that are meant to be called after the first call to initialize(...)

I think my biggest hangup is that separating out the initialization and reinitialization into two methods means there's a bigger chance that if we add new members to the class, we only add it to the initialization and not the reinitialization.

it would be even better to only allow initialize(...) to be called once

The way the code is written makes this a bit awkward. For example Modules is constructed once, but initialized every time we create a new inspector (e.g. hot restarts). We could have it maybe call the new reinitialize but then this method needs to make modifiedModuleReport nullable still because that's only relevant for hot reloads.

Copy link
Collaborator

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this LGTM with some style nits around the tests in particular.

I do have some concerns with some of the patterns we're using here, but I understand that we're working with the current structure of the project and I don't want to block this from getting into the next stable. I would like to spend some time in the (hopefully near) future and cleanup this codebase a bit.

@srujzs
Copy link
Contributor Author

srujzs commented Jul 4, 2025

Thanks Ben!

I would like to spend some time in the (hopefully near) future and cleanup this codebase a bit

One of the things I considered here as a future followup was to make many of these classes implement a Reloadable interface to make the initialization more consistent.

@srujzs srujzs merged commit b61423d into dart-lang:main Jul 4, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resetting of location and source information on hot reload should only invalidate the changed libraries
3 participants