Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Exclude string.ToLowerInvariant() in GetEnvironmentVariableAsBool()#14323

Merged
daxian-dbw merged 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:perf-converttobooliSazonov/PowerShell:perf-converttoboolCopy head branch name to clipboard
Jan 6, 2021
Merged

Exclude string.ToLowerInvariant() in GetEnvironmentVariableAsBool()#14323
daxian-dbw merged 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:perf-converttobooliSazonov/PowerShell:perf-converttoboolCopy head branch name to clipboard

Conversation

@iSazonov

@iSazonov iSazonov commented Dec 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

PR Summary

Exclude string.ToLowerInvariant() in GetEnvironmentVariableAsBool() to avoid early ICU initialization.

PR Context

Related #14268.

PR Checklist

@iSazonov iSazonov added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Dec 4, 2020
@iSazonov iSazonov mentioned this pull request Dec 4, 2020
Closed
15 tasks
@daxian-dbw

daxian-dbw commented Dec 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

@iSazonov I doubt the usefulness of this change. The most common scenario would be that the POWERSHELL_TELEMETRY_OPTOUT environment variable is not defined. The GetEnvironmentVariableAsBool method would just return from the if (string.IsNullOrEmpty(str)) block, never hitting the switch (str.ToLowerInvariant()) line.

Your changes here has zero impact to the most common scenario. It improves a less common scenario with the cost of decreased readability, so I'm not convinced we want it. @JamesWTruher, can you please review and share your thoughts?

@iSazonov

iSazonov commented Dec 4, 2020

Copy link
Copy Markdown
Collaborator Author

@daxian-dbw
As we already know, ApplicationInsights affects the startup speed unpredictably. So I turned off the telemetry locally to explore other areas.
The funny thing is that this code is the first thing I saw in the trace. It's a hindrance again.
Ideally we could use bool.Parse() method but it only support "true"/"false" strings.

@daxian-dbw

daxian-dbw commented Dec 4, 2020

Copy link
Copy Markdown
Member

@JamesWTruher Additional context info: string.ToLowerInvariant() triggers the loading of the International Component of Unicode package, which takes ~11ms in my local measurement.

@JamesWTruher

JamesWTruher commented Dec 4, 2020

Copy link
Copy Markdown
Collaborator

I'm somewhat in agreement with @daxian-dbw here. I don't see any actual measurement of performance change. If this is about improving performance, I would love to see what this actually gets us.

@iSazonov

iSazonov commented Dec 4, 2020

Copy link
Copy Markdown
Collaborator Author

I would like to avoid early loading of ICU. This is how it might look:
image

daxian-dbw
daxian-dbw requested changes Dec 5, 2020
View reviewed changes

@daxian-dbw daxian-dbw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@iSazonov I understand it's desired to avoid ToLowerInvariant. I think the concern is, the change improves a less common code path with the cost in readability. I will talk with @JamesWTruher more about it.

Comment thread src/System.Management.Automation/utils/Telemetry.cs Outdated
}

switch (str.ToLowerInvariant())
var boolStr = str.AsSpan().Trim();

@daxian-dbw daxian-dbw Dec 4, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a behavior change, that allows values like " yes " to work, which was discarded before.

@iSazonov iSazonov Dec 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This follow the intention to support many acceptable values.
But I wonder why we need this at all. We could support only "0" and "1" values or better to check only presents the variable as a flag.
If we simplify the rules we simplify the code.

@daxian-dbw daxian-dbw Dec 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't change behavior when doing perf optimization. Also, we cannot do breaking changes.

@iSazonov iSazonov Dec 5, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I removed the Trim().
Code below comes from .Net https://source.dot.net/#System.Private.CoreLib/Boolean.cs,198ff42f14d8c64b

I could make the method internal and add xUnit test for the method.

@daxian-dbw daxian-dbw Dec 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I removed the Trim().

@iSazonov Can you push your latest change?

@iSazonov iSazonov Dec 12, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh, sorry - forgot to push the commit.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Dec 5, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Dec 19, 2020
@ghost

ghost commented Dec 19, 2020

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

daxian-dbw
daxian-dbw reviewed Jan 5, 2021
View reviewed changes

@daxian-dbw daxian-dbw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@iSazonov Sorry for being sluggish. We can merge this one after you address the one more comment I left.

Comment thread src/System.Management.Automation/utils/Telemetry.cs
@iSazonov

iSazonov commented Jan 6, 2021

Copy link
Copy Markdown
Collaborator Author

Sorry for being sluggish.

Sometimes holidays take more energy than work 😆

@ghost ghost removed the Review - Needed The PR is being reviewed label Jan 6, 2021
@daxian-dbw daxian-dbw merged commit 6545689 into PowerShell:master Jan 6, 2021
@iSazonov iSazonov deleted the perf-converttobool branch January 6, 2021 18:32
@iSazonov iSazonov added this to the 7.2.0-preview.3 milestone Jan 6, 2021
@ghost

ghost commented Feb 12, 2021

Copy link
Copy Markdown

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SeeminglyScience SeeminglyScience SeeminglyScience left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

Assignees

@daxian-dbw 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.3

Development

Successfully merging this pull request may close these issues.

4 participants

@iSazonov @daxian-dbw @JamesWTruher @SeeminglyScience
Morty Proxy This is a proxified and sanitized view of the page, visit original site.