Skip to content

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

Merged
daxian-dbw merged 3 commits intoPowerShell:masterfrom
iSazonov:perf-converttobool
Jan 6, 2021
Merged

Exclude string.ToLowerInvariant() in GetEnvironmentVariableAsBool()#14323
daxian-dbw merged 3 commits intoPowerShell:masterfrom
iSazonov:perf-converttobool

Conversation

@iSazonov
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov commented Dec 4, 2020 •
edited
Loading

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
Copy link
Copy Markdown
Member

daxian-dbw commented Dec 4, 2020 •
edited
Loading

@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
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Dec 4, 2020

@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
Copy link
Copy Markdown
Member

daxian-dbw commented Dec 4, 2020

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

@JamesWTruher
Copy link
Copy Markdown
Collaborator

JamesWTruher commented Dec 4, 2020

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
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Dec 4, 2020

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
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

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.

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

switch (str.ToLowerInvariant())
var boolStr = str.AsSpan().Trim();
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Dec 4, 2020

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.

Copy link
Copy Markdown
Collaborator Author

@iSazonov iSazonov Dec 5, 2020 •
edited
Loading

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.

Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Dec 5, 2020

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.

Copy link
Copy Markdown
Collaborator Author

@iSazonov iSazonov Dec 5, 2020

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.

Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Dec 11, 2020

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?

Copy link
Copy Markdown
Collaborator Author

@iSazonov iSazonov Dec 12, 2020

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
Copy link
Copy Markdown

ghost commented Dec 19, 2020

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
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

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.

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Jan 6, 2021

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
Copy link
Copy Markdown

ghost commented Feb 12, 2021

🎉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