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

Add option to delay file system writes #3418

Closed
wants to merge 17 commits into from

Conversation

simolus3
Copy link
Contributor

@simolus3 simolus3 commented Dec 3, 2022

This adds a --delay-writes option to the build, watch, serve and daemon commands. When enabled, file system writes will only happen once at the end of a build.

We can determine the impact of this option on larger builds (in terms of how the analyzer reacts, memory used by the build system, ...). If it turns out that some refinements are needed (like only doing this for Dart files or source assets), we can improve the option in subsequent releases. Once everything works, we might want to make this the default mode (at least if not using the low resource mode).

This probably needs more integration tests, but I'm not sure what the place best to add them would be.

Closes #3321

@jakemac53
Copy link
Contributor

jakemac53 commented Dec 5, 2022

Can you explain a bit more the motivation for this change? Why does it make sense to delay the writes?

@jakemac53
Copy link
Contributor

Oh wait nvm, the linked issue, apparently filed by me, describes the reasoning, LOL.

build_runner/lib/src/generate/environment.dart Outdated Show resolved Hide resolved
build_runner_core/lib/src/asset/writer.dart Outdated Show resolved Hide resolved
build_runner_core/lib/src/asset/writer.dart Show resolved Hide resolved
build_runner_core/lib/src/asset/writer.dart Show resolved Hide resolved
build_runner_core/lib/src/asset/writer.dart Outdated Show resolved Hide resolved
build_runner_core/lib/src/generate/build_definition.dart Outdated Show resolved Hide resolved
build_runner_core/lib/src/generate/build_impl.dart Outdated Show resolved Hide resolved
@jakemac53
Copy link
Contributor

cc @natebosch did you want to review this as well?

Copy link
Member

@natebosch natebosch left a comment

Choose a reason for hiding this comment

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

I was initially worried, but I don't think it should be possible for this cache to have the same content as the CachingAssetReader - we should always invalidate an asset before it could be possible to write it, and we'd never read an asset from disk before writing it during the build.

});
bool? delayAssetWrites,
}) :
// Delayed asset writes should be enabled by default if we're not in the
Copy link
Member

Choose a reason for hiding this comment

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

How confident are we that this is the right choice for the default?

build_runner_core/lib/src/asset/writer.dart Show resolved Hide resolved
build_runner:
path: ../build_runner
build_runner_core:
path: ../build_runner_core
Copy link
Member

Choose a reason for hiding this comment

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

Please add a trailing newline

@jakemac53
Copy link
Contributor

new failure here looks potentially related

@simolus3
Copy link
Contributor Author

simolus3 commented Dec 19, 2022

I think that particular failure is from the InMemoryRunnerAssetWriter though: When it writes a makeAssetId('a|foo/bar.dart') on Windows, it notifies fake watchers about a write in D:\a\foo/bar.dart because it just takes the path of the asset id without modification. I think D:\a\foo\bar.dart would be the correct path in real builds.

Ah, already fixed in a better way by #3429 :D

@natebosch
Copy link
Member

Does the test pass if we revert the last commit "Fix symlink test" and merge master? I'd prefer to avoid the is check.

@simolus3
Copy link
Contributor Author

simolus3 commented Dec 30, 2022

Were you referring to b93d534? Unfortunately, createMergedOutputDirectories needs a path-providing reader when it wants to emit symlinks.

I agree that the is isn't great, should I move the interface of PathProvidingAssetReader into RunnerAssetReader (perhaps by encouraging throwing an UnsupportedError in most implementations)? That would make it a lot easier to delegate.

@natebosch
Copy link
Member

Sorry for the slow response.

Yes, I think merging the interface of PathProvidingAssetReader into RunnerAssetReader is sensible.

I'd guess few implementation won't be able to implement it, but I haven't looked closely.

@mosuem
Copy link
Member

mosuem commented Mar 12, 2024

@simolus3 is this still WIP?

Closing this for now as it is stale and the files are out of date. Feel free to reopen in case you still want to land it!

@mosuem mosuem closed this Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Add a mode where all actual file system interactions are delayed until the build completes
4 participants