Building M.RN takes longer and sometimes breaks incrementality

[asklar]

When we added PackageReference, we added two unconditional passes to restore every project upon every build first (we restore as PackageReference and then as packages.config).

This has 2 effects:

• makes the build slower: we end up spending ~30s-1m longer, just to get started on the real build, on every build
• it looks like sometimes msbuild writes despite nothing having changed; this breaks incremental builds, requiring rebuilding files that haven't changed

[chrisglein]

Sounds like the restore happening on every build (when it doesn't need to) is the first thing to look after.

[JunielKatarn]

Sounds like the restore happening on every build (when it doesn't need to) is the first thing to look after.

The slowdown issue is known. Because Visual Studio does not support PackageReference for C++, we explicitly run the Restore target when building under VS.

This is not an issue when building from the command line.

At the moment, I do not know of a way to detect whether the Restore target has been successfully run on a previous iteration.

[NickGerleman]

makes the build slower: we end up spending ~30s-1m longer, just to get started on the real build, on every build

This is much longer than I would expect restoration to take beyond the first build. Where are you seeing the time being taken?

Our build is unfortunately full of incrementality issues already. E.g. cppwinrt and autolinking. The precedent we have given so far is that it is okay not to be incremental, if the task is fast, and being incremental is "hard", but I would love to see us being clean for new changes.

@JunielKatarn do you think it would be possible to key the restoration target to only run if inputs change which could impact the restoration?

[NickGerleman]

Here's a quick reference on how MSBuild lets you do this normally: https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-build-incrementally?view=vs-2022

A trick we use sometimes when it is hard or expensive to look at outputs, is to do something like writing a file we control when we complete the target, then using the presence of that file and its timestamp as an indicator for whether we have been successful. E.g. for the packages.config case, you could declare the packages.config as an input, then the "status file" as an output. So that if the packages.config timestamp changes to something later than the file you wrote, MSBuild will rerun the task.

[NickGerleman]

Note that MSBuild uses timestamps instead of hashes for all of this. So, if you were to do something like copy an older packages.config file, MSBuild wouldn't in that case detect that there needs to be a change. @dannyvv would be a good expert to talk to about this sort of thing once his current work is a little less busy. I think this might be an MSBuild incrementality limitation in general though, where the general strategy of timestamp-based comparison it uses has some holes.

[JunielKatarn]

@JunielKatarn do you think it would be possible to key the restoration target to only run if inputs change which could impact the restoration?

That could definitely help, provided we already ensure that a non-incremental build will always do the full restore.

I'll read on that MSBuild link to learn how to track the changes.

Note, being a build performance issue, I'll be addressing any remaining build break corner cases first.

[JunielKatarn]

Is this still an issue after merging #9647?

[jonthysell]

@asklar Is what was done enough, or do we still want the incrementality?

@JunielKatarn Do you think incrementality would make 0.69 or should we move this to 0.70?

[asklar]

it's pretty slow in main, tbh. Much slower than before (about 30s at least). Do we have any recourse to e.g. at least do the restore in parallel for all projects or something? When doing F5 without changing anything, the build takes a long time just trying to restore (and not actually doing anything).

[jonthysell]

Moving to 0.70, we can take any perf improvement in this regard and do point releases back to 0.68, 0.69.

[chiaramooney]

@JunielKatarn Do we have an update here? Or should this issue be moved to 71?

[JunielKatarn]

@JunielKatarn Do we have an update here? Or should this issue be moved to 71?

This issue is still in the backlog due to prioritization. Let's punt to 71.

[Vyacheslav1557]

v74 and it is still very slow