Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Apr 10, 2018

Commit e3abe4b broke caching of the
bin/Build$(Configuration)/api/api-*.xml.* files, reintroducing the
scenario that commit 5c46ee3 fixed. The result is that minutes may
be spent needlessly re-generating API XML.

The cause of the breakage is that @(_ApiParameterDescription)
(introduced in e3abe4b) will contain entries that we don't care
about, e.g. API-4, for which no longer generate bindings. As such, the
_ClassParse target Inputs were looking for files which will never
exist, thus causing the _ClassParse target to always execute.

This could be fixed by making @(_ApiParameterDescription)/etc.
entries conditional on on the file
%(_ApiParameterDescription.ParameterDescription) refers to actually
existing, a'la:

<ItemGroup>
  <_ApiParameterDescription
      Condition="Exists('%(ApiFileDefinition.ParameterDescription)')"
      Include="%(ApiFileDefinition.ParameterDescription)"
  />
</ItemGroup>

However, this is suboptimal, as it means everything must still be
rebuilt if only one file changes:

$ touch src/Mono.Android/Profiles/api-27.params.txt
$ (cd build-tools/api-xml-adjuster ; time msbuild )
real    2m20.205s
# eep!

What we really want is MSBuild Target Batching, but our use of
item groups without item metadata prevents target batching!

Which leads to a realization: the rationalization for commit e3abe4b
was incomplete: The problem wasn't using metadata within Target Inputs
and Outputs; the problem was intermixing strings with metadata.

Meaning the pre-e3abe4b8 api-xml-adjuster.targets was correct
(except for the bit about it mentioning files which didn't exist,
causing needless rebuilds):

<!-- Good! Permits Target Batching -->
<Target Name="_ClassParse"
    Inputs="%(ApiFileDefinition.ParameterDescription)"
    Outputs="%(ApiFileDefinition.ApiAdjustedXml)"
  ...

vs. what e3abe4b was attempting to fix:

<!-- BAD! Intermixes metadata with strings -->
<Target Name="_Make"
    Inputs="$(IntermediateOutputPath)\%(_LibZipTarget.Identity)\Makefile"
  ...

Reviewing e3abe4b, api-xml-adjuster.targets is the only file that
was already properly using item medata within Target Inputs & Outputs.

Fix class-parse.exe and api-xml-adjuster.exe rebuilds by properly
using MSBuild Target Batching, and by filtering
@(ApiFileDefinition) so that it only contains values for which we
have a src/Mono.Android/Profiles/api-*.params.txt file.

With these changes in place, single-file rebuilds are much faster:

$ touch src/Mono.Android/Profiles/api-27.params.txt
$ (cd build-tools/api-xml-adjuster ; time msbuild )
real    0m26.662s

@jonpryor
Copy link
Contributor Author

@atsushieno: I reworked the patch here from the "simple" fix to something more resilient to rebuilds.

I can't find where you mentioned that touching e.g. src/Mono.Android/Profiles/api-27.params.txt would cause everything to be reconstructed, but that is not ideal. The updated PR addresses that, so that if the params for a single API changes, you only need to re-run class-parse and api-xml-adjuster.exe for that API level, and not everything.

@atsushieno
Copy link
Contributor

Yay, it is much better! (The problem was stated at the commit message in my former attempt to fix: f3f6005 )

@atsushieno
Copy link
Contributor

The build failed with:

03:38:16 	: error : Error building target _DefineApiFiles: Item has already been added. Key in dictionary: 'ParameterDescription'  Key being added: 'ParameterDescription'

Commit e3abe4b broke caching of the
`bin/Build$(Configuration)/api/api-*.xml.*` files, reintroducing the
scenario that commit 5c46ee3 fixed. The result is that *minutes* may
be spent needlessly re-generating API XML.

The cause of the breakage is that `@(_ApiParameterDescription)`
(introduced in e3abe4b) will contain entries that we don't care
about, e.g. API-4, for which no longer generate bindings. As such, the
`_ClassParse` target Inputs were looking for files which will never
exist, thus causing the `_ClassParse` target to always execute.

This *could* be fixed by making `@(_ApiParameterDescription)`/etc.
entries conditional on on the file
`%(_ApiParameterDescription.ParameterDescription)` refers to actually
existing, a'la:

	<ItemGroup>
	  <_ApiParameterDescription
	      Condition="Exists('%(ApiFileDefinition.ParameterDescription)')"
	      Include="%(ApiFileDefinition.ParameterDescription)"
	  />
	</ItemGroup>

However, this is suboptimal, as it means everything must still be
rebuilt if only one file changes:

	$ touch src/Mono.Android/Profiles/api-27.params.txt
	$ (cd build-tools/api-xml-adjuster ; time msbuild )
	real	2m20.205s
	# eep!

What we really want is [MSBuild Target Batching][0], but our use of
item groups *without* item metadata prevents target batching!

[0]: https://msdn.microsoft.com/en-us/library/ms171473.aspx

Which leads to a realization: the rationalization for commit e3abe4b
was incomplete: The problem wasn't using metadata within Target Inputs
and Outputs; the problem was intermixing strings with metadata.

Meaning the pre-e3abe4b8 `api-xml-adjuster.targets` was *correct*
(except for the bit about it mentioning files which didn't exist,
causing needless rebuilds):

	<!-- Good! Permits Target Batching -->
	<Target Name="_ClassParse"
	    Inputs="%(ApiFileDefinition.ParameterDescription)"
	    Outputs="%(ApiFileDefinition.ApiAdjustedXml)"
	  ...

vs. what e3abe4b was attempting to fix:

	<!-- BAD! Intermixes metadata with strings -->
	<Target Name="_Make"
	    Inputs="$(IntermediateOutputPath)\%(_LibZipTarget.Identity)\Makefile"
	  ...

Reviewing e3abe4b, `api-xml-adjuster.targets` is the only file that
was *already* properly using item medata within Target Inputs & Outputs.

Fix `class-parse.exe` and `api-xml-adjuster.exe` rebuilds by properly
using MSBuild Target Batching, *and* by *filtering*
`@(ApiFileDefinition)` so that it only contains values for which we
have a `src/Mono.Android/Profiles/api-*.params.txt` file.

With these changes in place, single-file rebuilds are much faster:

	$ touch src/Mono.Android/Profiles/api-27.params.txt
	$ (cd build-tools/api-xml-adjuster ; time msbuild )
	real	0m26.662s
@jonpryor jonpryor force-pushed the jonp-cache-api-xml branch from 25694ae to 355c5a2 Compare April 10, 2018 15:12
@jonpryor jonpryor merged commit 0c918cf into dotnet:master Apr 10, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants