Speedup InitialSessionState..cctor() - #14283
Conversation
| // with no content. So create a scratch file and test on that. | ||
| string dtAppLockerTestFileContents = AppLockerTestFileContents + DateTime.Now; | ||
| string dtAppLockerTestFileContents = AppLockerTestFileContents + DateTime.UtcNow.Ticks; | ||
| IO.File.WriteAllText(testPathScript, dtAppLockerTestFileContents); |
There was a problem hiding this comment.
It seems the file name does not depend on the DateTime, but rather the file contents. So maybe we want to keep human date format
There was a problem hiding this comment.
The file is created for automatic testing - no humans see and read the file.
There was a problem hiding this comment.
no humans see and read the file.
Perhaps it is for debugging purposes.
Actually, if no humans see the file do we require the datetime to be written in any form at all?
There was a problem hiding this comment.
I don't know. I am only resolving the perf issue. I can only guess it is how AppLocker works. Perhaps Travis Plunk (@TravisEz13) or Paul Higinbotham (@PaulHigin) could add more information.
There was a problem hiding this comment.
Travis Plunk (@TravisEz13) and Paul Higinbotham (@PaulHigin), can you please comment on whether we need DateTime to be added to the file content at all?
There was a problem hiding this comment.
The DateTime was included to add some randomness to the file contents. I think we still need it, but could change it to some other random value if computing it is quicker than DateTime.
There was a problem hiding this comment.
Ilya (@iSazonov) Maybe replacing DateTime.Now.Ticks with Environemnt.TickCount?
There was a problem hiding this comment.
Dongbo Wang (@daxian-dbw) I have no objections. I see Environment.TickCount64 - maybe use it?
There was a problem hiding this comment.
That sounds fine to me, go ahead.
There was a problem hiding this comment.
Done. The PR description has been updated too.
| // AppLocker fails when you try to check a policy on a file | ||
| // with no content. So create a scratch file and test on that. | ||
| string dtAppLockerTestFileContents = AppLockerTestFileContents + DateTime.Now; | ||
| string dtAppLockerTestFileContents = AppLockerTestFileContents + DateTime.UtcNow.Ticks; |
There was a problem hiding this comment.
| string dtAppLockerTestFileContents = AppLockerTestFileContents + DateTime.UtcNow.Ticks; | |
| string dtAppLockerTestFileContents = AppLockerTestFileContents + DateTime.UtcNow.ToString("u"); |
There was a problem hiding this comment.
I took real world measurements using PerfView 😋
It seems my suggested change is not much of an improvement, I measured 8ms for the code line.
I also measured the original code line and it was 14ms, same as your measurement.
There was a problem hiding this comment.
DateTime.UtcNow is more fast - it does not read current user time zone and does not convert to local time.
Is this a release build? Running a microbenchmark, |
Yes, it is release build. And it is startup scenario - cold start (.Net runtime loads Calendar data from OS and initializes internal structures - you can see this in attached trace file.). If you run BenchmarkDotNet it is warm start. |
|
🎉 Handy links: |
… seed for AppLocker test file content (PowerShell#14283)
PR Summary
At startup we create a temp file with name based on
Datetime.Now.ToString().It is very slow - ~14ms.
We have no need to use human date format for the temp file.
The fix is to use
Datetime.UtcNow().Ticks.ToString()Environment.TickCount64:Environment.TickCount64is significantly faster Datetime.Now() - it is intrinsic and no need to evaluate local time.Perf win is ~10-30 ms.
Before
After
PR Context
Tracking issue #14268
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.