Skip to content

Conversation

@jashook
Copy link
Contributor

@jashook jashook commented May 15, 2020

Closes #33438

@ghost
Copy link

ghost commented May 15, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@jashook jashook requested review from echesakov and trylek May 15, 2020 20:21
@jaredpar
Copy link
Member

How much size does this end up saving?

@jashook
Copy link
Contributor Author

jashook commented May 15, 2020

I will post size and time saved once the runs finish.

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

If the intent here to remove ALL pdb from the Helix submission then you don't need to change testgrouping.proj This file should be left for specifying partitioning of the test tree.

Also, some of the test tree subdirectories are not included in TestGrouping so you would miss pdb-s in those.

What you want instead is to add

  <_PayloadFiles Remove="*.pdb" />

at the end of this ItemGroup

<ItemGroup>
<!-- If no TestGrouping is defined, all the files under $(_FileDirectory) and its subdirectories goes to the PayloadDirectory. -->
<_PayloadFiles Include="$(_FileDirectory)**" Exclude="@(_TestGroupingRelevant)" Condition="'$(_TestGroupingExists)' != 'true'" />
<!-- If there is a TestGrouping, then take only the files that belong to the TestGroup == $(_PayloadGroup). -->
<_PayloadFiles Include="@(_TestGroupingRelevant->WithMetadataValue('TestGroup','$(_PayloadGroup)')->DistinctWithCase())" Condition="'$(_TestGroupingExists)' == 'true'" />
<_PayloadFiles Include="$(_FileDirectory)*" Condition="'$(_TestGroupingExists)' == 'true'" />
<_PayloadFiles Update="@(_PayloadFiles)">
<!-- Never use [MSBuild]::MakeRelative here! We have some files containing Unicode characters in their %(FullPath) and
MakeRelative function calls Escape function internally that replaces all the Unicode characters with %<xx>. -->
<FileRelativeToPayloadsRootDirectory>$(_PayloadGroup)\$([System.IO.Path]::GetRelativePath($(BinDir), %(FullPath)))</FileRelativeToPayloadsRootDirectory>
</_PayloadFiles>
</ItemGroup>

@jashook jashook force-pushed the exclude-managed-pdbs branch from e5cfac4 to 65859c5 Compare May 15, 2020 22:02
@jashook
Copy link
Contributor Author

jashook commented May 22, 2020

Looks like this did not correctly remove pdbs. Looking further into it.

@jashook
Copy link
Contributor Author

jashook commented May 23, 2020

After way more time than I wanted to spend looking at msbuild logs, I believe this correctly filters out the managed pdbs. I added removing the .sh/.cmd files on the perspective targets while I was there. Although the size difference for scripts is extremely small.

@echesakov
Copy link
Contributor

After way more time than I wanted to spend looking at msbuild logs, I believe this correctly filters out the managed pdbs. I added removing the .sh/.cmd files on the perspective targets while I was there. Although the size difference for scripts is extremely small.

Yeah, MSBuild debugging is painful. Have you figured out why my approach didn't work out?

@jashook
Copy link
Contributor Author

jashook commented May 23, 2020

I cannot explain why remove failed to remove... I came to the (possibly incorrect assumption) that you cannot use a wildcard to remove from a list.

@jashook
Copy link
Contributor Author

jashook commented May 23, 2020

Its quite unfortunate as well because your proposed solution was quite clean.

@echesakov
Copy link
Contributor

I cannot explain why remove failed to remove... I came to the (possibly incorrect assumption) that you cannot use a wildcard to remove from a list.

No, you can usually do this. See https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-items?view=vs-2019#BKMK_RemoveAttribute

But I remember observing a similar behavior in the past...

Maybe @sbomer knows why? I put a link to the change with wrong behavior - 65859c5

@jashook
Copy link
Contributor Author

jashook commented May 23, 2020

I was looking at the same thing earlier, it could be a bug with removing things with Attributes?

@jashook
Copy link
Contributor Author

jashook commented May 23, 2020

I do not have timing information yet, but this change reduces the aggregate size of our pri0 Windows payload from 1.92 gb to 313 mb.

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jashook !

Changes look good - I don't think it's worth spending more time to figure out why Remove-attribute doesn't work (even though I would love to know why)

At some point we need to collect information how this affects the Helix setup time.

@jashook
Copy link
Contributor Author

jashook commented May 23, 2020

I will have that information soon. I am currently a few days behind, but I believe I should ingest up to today over the long weekend. Will post with timing then.

https://gist.github.com/jashook/42fb536682ac34f032f1bc095397c59b

@sbomer
Copy link
Member

sbomer commented May 23, 2020

I haven't confirmed, but I think the wildcard matches against files in the current directory only, not all members of the ItemGroup. **/*.pdb may work.

@jashook jashook force-pushed the exclude-managed-pdbs branch from 617a17f to bfeb333 Compare June 5, 2020 18:10
@jashook
Copy link
Contributor Author

jashook commented Jun 6, 2020

Failures are unrelated. Data I hard to show without more runs as networking gives a lot of noise. However, rough estimate is roughly 30-50% improvement.

@jashook jashook merged commit 8f1c72d into dotnet:master Jun 6, 2020
@jashook jashook deleted the exclude-managed-pdbs branch June 6, 2020 00:03
@jashook
Copy link
Contributor Author

jashook commented Jun 11, 2020

After ~1 week of data collection there is enough data to show this change reduced helix setup time for coreclr and mono submissions by ~50%.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prune all helix payloads

5 participants