Webcmdlets cleanup #19308

CarloToso · 2023-03-10T12:19:58Z

PR Summary

Replace var
Use GeneratedRegex
Remove EnsureHtmlParser() (no longer needed)
Remove unneeded code (we already use UTF8 as default)

                if (string.IsNullOrEmpty(characterSet) && ContentHelper.IsJson(contentType))
                {
                    characterSet = Encoding.UTF8.HeaderName;
                }

PR Context

PR Checklist

iSazonov · 2023-03-10T17:00:40Z

...hell.Commands.Utility/commands/utility/WebCmdlet/Common/BasicHtmlWebResponseObject.Common.cs

-        private static Regex s_linkRegex;
-        private static Regex s_tagRegex;
+        [GeneratedRegex(@"([^""'>/=\s\p{Cc}]+)(?:\s*=\s*(?:""(.*?)""|'(.*?)'|([^'"">\s]+)))?", RegexOptions.Singleline | RegexOptions.IgnoreCase)]
+        private static partial Regex s_attribNameValueRegex();


I don't see the point of this change. The original Regex is compiled and works just as fast, but it is more compact.

Thanks to this change we can remove EnsureHtmlParser();

And what are the benefits? The final code is still noticeably larger, but the performance does not increase.

I will revert the regex stuff, move the remaining changes to the other webcmdlet cleanup PR and close this PR

There's significant difference between RegexOptions.Compiled and GeneratedRegex. Regex Source Generator will not only reduce startup time but also increase performance.
I can't simply put it into words how much better generated regex is, thankfully Stephen Toub has great post about them here https://devblogs.microsoft.com/dotnet/regular-expression-improvements-in-dotnet-7/#source-generation (In fact, .NET team wants people to switch to GeneratedRegex so bad that they want to intercept every new Regex(..) call and delegate them to source generated one instead)

Stephen also talks about them here https://www.youtube.com/live/rwfNDyBBgks?t=1549
Nick Chapsas has also a quick summary of them https://www.youtube.com/watch?v=WosEhlHATOk

I can't simply put it into words how much better generated regex is,

When it comes to code performance neither words nor emotions work - you have to prove with tests that a particular code will become smaller and faster.

In this particular case, the code will only get bigger because PowerShell is dynamic and can never completely get rid of the Regex compiler. The initial improvements won't be noticeable since the initialization of the cmdlet takes a significant amount of time.

Where we would like Regex SG and where it is justified is the pwsh startup scenario. And there are indeed places where this SG can be applied.

@iSazonov if you pointed me to such cases I could try to apply Regex SG

When it comes to code performance neither words nor emotions work - you have to prove with tests that a particular code will become smaller and faster.

You're correct here. I should have provided data for my claims.

In this particular case, the code will only get bigger because PowerShell is dynamic and can never completely get rid of the Regex compiler. The initial improvements won't be noticeable since the initialization of the cmdlet takes a significant amount of time.

Where we would like Regex SG and where it is justified is the pwsh startup scenario. And there are indeed places where this SG can be applied.

I didn't realize this code wasn't used in the pwsh itself, I admit my mistake. In this case delaying parsing and doing all the complex IL emitting is justified, if this code is not called in a cmdlet.

This also makes me want to compare the size of the IL emitted for RegexOptions.Compiled and the IL compiled by Roslyn for the source generated one.. interesting.

pull-request-quantifier-deprecated · 2023-03-10T17:49:58Z

This PR has 76 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Small
Size       : +28 -48
Percentile : 30.4%

Total files changed: 2

Change summary by file extension:
.cs : +28 -48

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

SavageHaxCash · 2023-03-11T13:09:24Z

this was a huge dump of different files and programs to see if it could be handled thanks for your feedback !!!Sent from my iPhoneOn Mar 10, 2023, at 12:02 PM, Ilya ***@***.***> wrote: @iSazonov commented on this pull request. In src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/BasicHtmlWebResponseObject.Common.cs:

{

#region Private Fields - private static Regex s_attribNameValueRegex; - private static Regex s_attribsRegex; - private static Regex s_imageRegex; - private static Regex s_inputFieldRegex; - private static Regex s_linkRegex; - private static Regex s_tagRegex; + [GeneratedRegex(@"([^""'>/=\s\p{Cc}]+)(?:\s*=\s*(?:""(.*?)""|'(.*?)'|([^'"">\s]+)))?", RegexOptions.Singleline | RegexOptions.IgnoreCase)] + private static partial Regex s_attribNameValueRegex(); I don't see the point of this change. The original Regex is compiled and works just as fast, but it is more compact. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

CarloToso added 3 commits March 10, 2023 12:49

remove useless utf8

97d8381

use GeneratedRegex

b1a218d

remove var

53472a1

CarloToso requested a review from PaulHigin as a code owner March 10, 2023 12:19

ghost assigned iSazonov Mar 10, 2023

pull-request-quantifier-deprecated bot added the Small label Mar 10, 2023

CarloToso added 2 commits March 10, 2023 14:24

remove this

6ff95b4

Streamhelper generatedRegex

3654a56

CarloToso changed the title ~~BasicHtmlWebResponseObject.Common.cs cleanup~~ Webcmdlets cleanup Mar 10, 2023

iSazonov reviewed Mar 10, 2023

View reviewed changes

use TryGetEncoding

e758f02

CarloToso added a commit to CarloToso/PowerShell that referenced this pull request Mar 10, 2023

changes from PowerShell#19308

c820546

CarloToso mentioned this pull request Mar 10, 2023

Small cleanup WebCmdlets #19299

Merged

22 tasks

CarloToso closed this Mar 10, 2023

CarloToso deleted the basichtmlwebresponse-regex branch March 10, 2023 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Webcmdlets cleanup #19308

Webcmdlets cleanup #19308

Uh oh!

CarloToso commented Mar 10, 2023

Uh oh!

iSazonov Mar 10, 2023

Uh oh!

CarloToso Mar 10, 2023

Uh oh!

iSazonov Mar 10, 2023

Uh oh!

CarloToso Mar 10, 2023 •

edited

Loading

Uh oh!

PaulusParssinen Mar 11, 2023 •

edited

Loading

Uh oh!

iSazonov Mar 11, 2023

Uh oh!

CarloToso Mar 12, 2023

Uh oh!

PaulusParssinen Mar 12, 2023

Uh oh!

pull-request-quantifier-deprecated bot commented Mar 10, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

SavageHaxCash commented Mar 11, 2023 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Webcmdlets cleanup #19308

Webcmdlets cleanup #19308

Uh oh!

Conversation

CarloToso commented Mar 10, 2023

PR Summary

PR Context

PR Checklist

Uh oh!

iSazonov Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

CarloToso Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

iSazonov Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

CarloToso Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulusParssinen Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Mar 11, 2023

Choose a reason for hiding this comment

Uh oh!

CarloToso Mar 12, 2023

Choose a reason for hiding this comment

Uh oh!

PaulusParssinen Mar 12, 2023

Choose a reason for hiding this comment

Uh oh!

pull-request-quantifier-deprecated bot commented Mar 10, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

SavageHaxCash commented Mar 11, 2023 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CarloToso Mar 10, 2023 •

edited

Loading

PaulusParssinen Mar 11, 2023 •

edited

Loading