Add some optimizations in formatting subsystem #6678

iSazonov · 2018-04-19T07:23:24Z

PR Summary

Continue #6648
Use safe pattern 'columns <= columnsThresHold ? stackalloc int[columns] : new int[columns];'.
Remove duplicate code (we already have it in GetConsoleWindowWidth() with a cache).
Use common 'StackAllocThreshold ' constant.

PR Checklist

Use safe pattern 'columns <= columnsThresHold ? stackalloc int[columns] : new int[columns];' Remove duplicate code. Use common 'columnsThresHold' constant

daxian-dbw · 2018-04-23T19:04:23Z

src/System.Management.Automation/FormatAndOutput/common/BaseOutputtingCommand.cs

        /// </summary>
        private enum PreprocessingState { raw, processed, error }

+        const int defaultConsoleWidth = 120;


This variable is only used in the method GetConsoleWindowWidth. Maybe it should be kept as a local const variable in that method.

I know both const vars here are with the value 120, but the naming suggests different things. return defaultConsoleWidth in GetConsoleWindowWidth is more readable than return StackAllocThreshold in my opinion :)

I put both constants next to each other specially because if we decide to change the first value, we probably want to change the second value too.

I added the private modifier to 'DefaultConsoleWidth'.

daxian-dbw · 2018-04-23T19:06:26Z

src/System.Management.Automation/FormatAndOutput/common/BaseOutputtingCommand.cs

        private enum PreprocessingState { raw, processed, error }

+        const int defaultConsoleWidth = 120;
+        internal const int columnsThresHold = 120;


columnsThresHold

threshold is a single word, so maybe change it to columnsThreshold?

This threshold is more about using stack allocation or managed heap allocation, so maybe we can give it a more general name, like StackAllocThreshold.

As for the naming, you probably should use StackAllocThreshold, accoding to the naming convention code-guidline

Use PascalCase to name constant local variables and fields. The only exception is for interop code where the constant should exactly match the name and value of the code you are calling via interop (e.g. const int ERROR_SUCCESS = 0).

daxian-dbw · 2018-04-23T19:23:11Z

src/System.Management.Automation/FormatAndOutput/common/ListWriter.cs

        /// <param name="dc">instance of the DisplayCells helper object</param>
        internal void Initialize(string[] propertyNames, int screenColumnWidth, DisplayCells dc)
        {
+            const int propertiesThresHold = OutCommandInner.columnsThresHold;


I don't think you need this; just use OutCommandInner.columnsThresHold instead.
C# compiler will use the int value directly in the IL. See an example here:

C# code

void Main() { int a = Console.Read(); string[] b = a <= Test.columnsThresHold ? new string[2] : new string[3]; } // Define other methods and classes here internal class Test { internal const int columnsThresHold = 120; }

IL code

IL_0000: nop IL_0001: call System.Console.Read IL_0006: stloc.0 // a IL_0007: ldloc.0 // a IL_0008: ldc.i4.s 78 // <=== 0x78 == 120 IL_000A: ble.s IL_0014 IL_000C: ldc.i4.3 IL_000D: newarr System.String IL_0012: br.s IL_001A IL_0014: ldc.i4.2 IL_0015: newarr System.String IL_001A: stloc.1 // b IL_001B: ret Test..ctor: IL_0000: ldarg.0 IL_0001: call System.Object..ctor IL_0006: nop IL_0007: ret

Thanks for the in-depth explanation!

Fixed.

daxian-dbw · 2018-04-23T19:23:34Z

src/System.Management.Automation/FormatAndOutput/common/TableWriter.cs

-                {
-                    screenColumns = 120;
-                }
-            }


Why are we removing this part?

It duplicates a code in GetConsoleWindowWidth so we should remove this (or add cache as in GetConsoleWindowWidth).

OK, got it. #close

daxian-dbw · 2018-04-23T19:24:24Z

src/System.Management.Automation/FormatAndOutput/common/TableWriter.cs


        private int _startColumn = 0;
+
+        const int columnsThresHold = OutCommandInner.columnsThresHold;


Same here. No need to declare another const int variable.

iSazonov · 2018-04-25T08:23:50Z

@daxian-dbw Thanks for help!

Add some optimizations in formatting subsystem

03b37d5

Use safe pattern 'columns <= columnsThresHold ? stackalloc int[columns] : new int[columns];' Remove duplicate code. Use common 'columnsThresHold' constant

iSazonov self-assigned this Apr 19, 2018

iSazonov requested review from SteveL-MSFT, TravisEz13 and daxian-dbw April 19, 2018 07:23

daxian-dbw requested changes Apr 23, 2018

View reviewed changes

iSazonov and others added 2 commits April 24, 2018 09:55

Address feedback

8ce481c

Add the modifier for 'DefaultConsoleWidth'

c5c04e0

daxian-dbw approved these changes Apr 24, 2018

View reviewed changes

iSazonov merged commit a9781de into PowerShell:master Apr 25, 2018

iSazonov deleted the span-listwriter branch April 25, 2018 08:23


		private int _startColumn = 0;

		const int columnsThresHold = OutCommandInner.columnsThresHold;

Add some optimizations in formatting subsystem #6678

Add some optimizations in formatting subsystem #6678

Uh oh!

Conversation

iSazonov commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Apr 23, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

C# code

IL code

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Apr 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iSazonov commented Apr 19, 2018 •

edited

Loading

daxian-dbw Apr 23, 2018 •

edited

Loading

daxian-dbw Apr 23, 2018 •

edited

Loading