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

[release/5.0-rc2] Revert #40641 #41820

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 3, 2020

Backport of #41817 to release/5.0-rc2

/cc @carlossanlop

Customer Impact

Perf regression as described in detail in #41739.

PR #40641 made a change to “Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way.” This fixed an issue discovered in PowerShell that “FileSystemEntry.Attributes property is not correct on Unix.” The change was made in 5.0 RC1 but our perf runs identified the regression that the fix introduced. We seek to revert that PR, reintroducing the bug, but eliminating the perf regression. We will then address the initial bug with a different approach in 6.0.

Testing

The affected benchmark was run with and without this change, which showed a 4.37 perf regression when comparing results (before = without the bug fix, after = with the bug fix):

summary:
worse: 1, geomean: 4.371
total diff: 1

| Slower                                        | diff/base | Base Median (ns) | Diff Median (ns) | Modality|
| --------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.IO.Tests.Perf_Directory.EnumerateFiles |      4.37 |       3996385.16 |      17467169.23 |         |

No Faster results for the provided threshold = 0.001% and noise filter = 0.3ns.

Risk

The code fix will be reverted so the bug will be brought back, but we will not have a negative performance impact in 5.0. Reintroducing the bug will affect PowerShell (where #37301 was first discovered). We confirmed with @SteveL-MSFT that this is acceptable; the bug will remain a known issue in 5.0.

Reverts "Ensure FileStatus and FileSystemEntry Hidden and ReadOnly attributes are retrieved the same way"
(commit 6ed1e41) which introduced a perf regression.
@danmoseley
Copy link
Member

@jeffhandley LGTM please mail tactics if you didn’t already

@carlossanlop
Copy link
Member

carlossanlop commented Sep 3, 2020

Approved by tactics too via email, @danmosemsft . Just need a sign-off.

@jeffhandley jeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 3, 2020
@jeffhandley
Copy link
Member

I've signed off and updated the labels per Dan's previous comment and Tactics approval. Feel free to merge, @carlossanlop.

@carlossanlop carlossanlop merged commit feefe4e into release/5.0-rc2 Sep 3, 2020
@carlossanlop carlossanlop deleted the backport/pr-41817-to-release/5.0-rc2 branch September 3, 2020 21:12
@preethikurup preethikurup added this to In progress in .NET Core impacting internal partners via automation Sep 22, 2020
@danmoseley danmoseley moved this from In progress to Done in .NET Core impacting internal partners Nov 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO Servicing-approved Approved for servicing release
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants