GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Speedup InitialSessionState..cctor() by iSazonov · Pull Request #14283 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Nov 28, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.TickCount64 is significantly faster Datetime.Now() - it is intrinsic and no need to evaluate local time.
  • Ticks is long type and Environment.TickCount64.ToString() is very fast. Datetime.ToString() is slow because it involves Calendar.

Perf win is ~10-30 ms.

Before
image

After
image

PR Context

Tracking issue #14268

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • Breaking changes
    • None
    • OR
    • Experimental feature(s) needed
      • Experimental feature name(s):
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
  • Tooling
    • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
    • OR
    • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
      • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
      • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
Speedup InitialSessionState..cctor()
da0aefe
@iSazonov Ilya (iSazonov) added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Nov 28, 2020
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Dongbo Wang (daxian-dbw) November 28, 2020 20:23
@iSazonov Ilya (iSazonov) assigned Dongbo Wang (daxian-dbw) Nov 28, 2020
@iSazonov
Ilya (iSazonov) requested a review from Travis Plunk (TravisEz13) as a code owner November 28, 2020 20:23
@iSazonov Ilya (iSazonov) mentioned this pull request Nov 28, 2020
Startup performance PowerShell 7.2.0-Preview.1 #14268
Closed
12 of 15 tasks
xtqqczze
xtqqczze reviewed Nov 29, 2020
View reviewed changes
Comment thread src/System.Management.Automation/security/wldpNativeMethods.cs
// 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);

@xtqqczze xtqqczze Nov 29, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 29, 2020

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The file is created for automatic testing - no humans see and read the file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@xtqqczze xtqqczze Nov 29, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 29, 2020

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Dec 2, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@PaulHigin Paul Higinbotham (PaulHigin) Dec 2, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Dec 3, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ilya (@iSazonov) Maybe replacing DateTime.Now.Ticks with Environemnt.TickCount?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Dec 4, 2020

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Dongbo Wang (@daxian-dbw) I have no objections. I see Environment.TickCount64 - maybe use it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Dec 4, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That sounds fine to me, go ahead.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@iSazonov Ilya (iSazonov) Dec 4, 2020

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done. The PR description has been updated too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
xtqqczze
xtqqczze reviewed Nov 29, 2020
View reviewed changes
Comment thread src/System.Management.Automation/security/wldpNativeMethods.cs Outdated
// 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;

@xtqqczze xtqqczze Nov 29, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
string dtAppLockerTestFileContents = AppLockerTestFileContents + DateTime.UtcNow.Ticks;
string dtAppLockerTestFileContents = AppLockerTestFileContents + DateTime.UtcNow.ToString("u");

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@xtqqczze xtqqczze Nov 29, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 30, 2020

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

DateTime.UtcNow is more fast - it does not read current user time zone and does not convert to local time.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
xtqqczze reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@iSazonov
Ilya (iSazonov) requested a review from Paul Higinbotham (PaulHigin) November 29, 2020 16:29
@xtqqczze

xtqqczze commented Nov 29, 2020

Copy link
Copy Markdown
Contributor

It is very slow - ~14ms.

Is this a release build?

Running a microbenchmark, Datetime.Now.ToString() takes ~1us!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 29, 2020

Copy link
Copy Markdown
Collaborator Author

It is very slow - ~14ms.

Is this a release build?

Running a microbenchmark, Datetime.Now.ToString() takes ~1us!

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.
xtqqczze reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Use Environment.TickCount64
766efc6
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Dec 4, 2020
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit ea3036d into PowerShell:master Dec 4, 2020
@daxian-dbw Dongbo Wang (daxian-dbw) added this to the 7.2.0-preview.2 milestone Dec 4, 2020
@iSazonov
Ilya (iSazonov) deleted the perf-startup-datetime branch December 4, 2020 17:28
@ghost

Deleted user (ghost) commented Dec 15, 2020

Copy link
Copy Markdown

🎉v7.2.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

  • Release Notes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov
Use Environment.TickCount64 instead of Datetime.Now as the random…
1b51898 
… seed for AppLocker test file content (PowerShell#14283)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@TravisEz13 Travis Plunk (TravisEz13) Awaiting requested review from TravisEz13 TravisEz13 is a code owner
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
+1 more reviewer
@xtqqczze xtqqczze xtqqczze left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log

Projects

None yet

Milestone

7.2.0-preview.2

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@iSazonov @xtqqczze @daxian-dbw @PaulHigin
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.