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

fasta incremental compiler need to update list of sources as imports are removed #36197

Open
aam opened this Issue Mar 13, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@aam
Copy link
Contributor

aam commented Mar 13, 2019

When using incremental compiler currently there seems to be no way to know that source is no longer used if last import of that source was removed from the compiled application.

https://dart-review.googlesource.com/c/sdk/+/96828 has a test.

cc @peter-ahe-google @jensjoha

@aam aam added the front-end-fasta label Mar 13, 2019

@jensjoha

This comment has been minimized.

Copy link
Contributor

jensjoha commented Mar 14, 2019

The change "needing" this seems to be https://dart-review.googlesource.com/c/sdk/+/95920.

There seems to be a weird assumption here, that the uriToSource on a component is exactly what you need it to be for your usecase: namely it should contain exactly the sources (no more, no less) the full component has - also if it's a partial/incremental component. That seems like a weird assumption to me.

It's also not that simple.

The incremental compiler keeps the builders for packages around - even after they're not referenced anymore. This is done for efficiency - either if the user flip-flops between including a package or not, or (and this is why it as done) for compiling tests, where not every flutter test actually depend on the flutter package, but where unloading all of flutter only to have to compile it again on the next test compilation seems like a waste of time.
For proper invalidation, what you would actually need is the list of sources the incremental compiler currently keeps around (as well as the .package file).

My vote would definitely be to keep it as-is, and only send an ever-growing list of uris to be monitored for changes. In practice it will be pruned when you restart the incremental compiler (when writing a dill the sources are pruned to only include the necessary, so when initializing from dill (or starting all over for that matter) we get a "fresh start").

@aam

This comment has been minimized.

Copy link
Contributor Author

aam commented Mar 14, 2019

There seems to be a weird assumption here, that the uriToSource on a component is exactly what you need it to be for your usecase: namely it should contain exactly the sources (no more, no less) the full component has - also if it's a partial/incremental component. That seems like a weird assumption to me.

Clients currently use uriToSource when generating "deps" file for the kernel dill file - they have been relying on that for a while now.
Extending this to incremental use case seems logical: uriToSource still maintains reliable sources list even during incremental updates. If there is another way or mechanism to provide up-to-date list of sources that is used by compiler, clients can switch to that.

My vote would definitely be to keep it as-is, and only send an ever-growing list of uris to be monitored for changes

Ultimately is the reason for this technical/performance related to flutter test use case or you think it's conceptually right thing to do? If former, then perhaps we could disable uriToSource updating mechanism for that use case: flutter test is not going to use those dependencies.

@jensjoha

This comment has been minimized.

Copy link
Contributor

jensjoha commented Mar 18, 2019

There seems to be a weird assumption here, that the uriToSource on a component is exactly what you need it to be for your usecase: namely it should contain exactly the sources (no more, no less) the full component has - also if it's a partial/incremental component. That seems like a weird assumption to me.

Clients currently use uriToSource when generating "deps" file for the kernel dill file - they have been relying on that for a while now.
Extending this to incremental use case seems logical: uriToSource still maintains reliable sources list even during incremental updates. If there is another way or mechanism to provide up-to-date list of sources that is used by compiler, clients can switch to that.

I'm not saying don't use it for deps (actually I'm not really sure how I feel about that) - I'm saying don't assume it should fit what you hoped exactly, and that what you seemingly propose certainly doesn't make more sense to me that the current thing.

My vote would definitely be to keep it as-is, and only send an ever-growing list of uris to be monitored for changes

Ultimately is the reason for this technical/performance related to flutter test use case or you think it's conceptually right thing to do? If former, then perhaps we could disable uriToSource updating mechanism for that use case: flutter test is not going to use those dependencies.

To me it seems perfectly valid for the incremental compiler to keep things around despite not technically being needed. As such those files also has to be invalidated upon change.

That being said, we do remove some libraries (the un-referenced non-package ones and the ones that are invalidated) - and we might not clean up the uriToSource for that. I'll fix that.

@jensjoha

This comment has been minimized.

Copy link
Contributor

jensjoha commented Mar 18, 2019

https://dart-review.googlesource.com/c/sdk/+/97201 should make it so only the libraries we keep around are included in uriToSource, but where we still keep around packages if they are "referenced" by the .packages file.

@jensjoha

This comment has been minimized.

Copy link
Contributor

jensjoha commented Mar 21, 2019

With eb344e2 landed, what are your feelings on this issue?

@aam

This comment has been minimized.

Copy link
Contributor Author

aam commented Mar 21, 2019

With eb344e2 landed, what are your feelings on this issue?

Thanks for landing that, I think we could close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.