Skip to content

Formatting: handle XTPUSHSGR/XTPOPSGR sequences #10208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 4 commits into from
Aug 16, 2019
Merged

Formatting: handle XTPUSHSGR/XTPOPSGR sequences #10208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a09517
Formatting: handle XTPUSHSGR/XTPOPSGR sequences
jazzdelightsme Jul 23, 2019
8f4878e
PR auto feedback: address style
jazzdelightsme Jul 23, 2019
254766f
address feedback from iSazonov
jazzdelightsme Jul 25, 2019
a3d4f12
Remove duplicate comment
daxian-dbw Aug 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 70 additions & 17 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2666,38 +2666,91 @@ internal static void SetConsoleTextAttribute(ConsoleHandle consoleHandle, WORD a
// Return the length of a VT100 control sequence character in str starting
// at the given offset.
//
// This code only handles the most common formatting sequences, which are
// all of the pattern:
// ESC '[' digits+ (';' digits)* 'm'
// This code only handles the following formatting sequences, corresponding to
// the patterns:
// CSI params? 'm' // SGR: Select Graphics Rendition
// CSI params? '#' [{}pq] // XTPUSHSGR ('{'), XTPOPSGR ('}'), or their aliases ('p' and 'q')
//
// There are many other VT100 escape sequences, but this simple pattern
// is sufficient for our formatting system. We won't handle cursor movements
// or other attempts at animation.
// Where:
// params: digit+ (';' params)?
// CSI: C0_CSI | C1_CSI
// C0_CSI: \x001b '[' // ESC '['
// C1_CSI: \x009b
//
// Note that offset is adjusted past the escape sequence.
// There are many other VT100 escape sequences, but these text attribute sequences
// (color-related, underline, etc.) are sufficient for our formatting system. We
// won't handle cursor movements or other attempts at animation.
//
// Note that offset is adjusted past the escape sequence, or at least one
// character forward if there is no escape sequence at the specified position.
internal static int ControlSequenceLength(string str, ref int offset)
{
var start = offset;
if (str[offset++] != (char)0x1B)

// First, check for the CSI:
if ((str[offset] == (char)0x1b) && (str.Length > (offset + 1)) && (str[offset + 1] == '['))
{
// C0 CSI
offset += 2;
}
else if (str[offset] == (char)0x9b)
{
// C1 CSI
offset += 1;
}
else
{
// No CSI at the current location, so we are done looking, but we still
// need to advance offset.
offset += 1;
return 0;
}

if (offset >= str.Length || str[offset] != '[')
if (offset >= str.Length)
{
return 0;
}

offset += 1;
while (offset < str.Length)
// Next, handle possible numeric arguments:
char c;
do
{
var c = str[offset++];
if (c == 'm')
break;
c = str[offset++];
Copy link
Collaborator

@iSazonov iSazonov Jul 24, 2019

Choose a reason for hiding this comment

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

I'd use a temp variable in the cycle for performance instead of offset which is a reference.

           char c; 
           int i = offset; 
           do 
           { 
               c = str[i++];
           }
           while ((i < str.Length) && (char.IsDigit(c) || c == ';'));
           offset = i;

Copy link
Contributor Author

@jazzdelightsme jazzdelightsme Jul 25, 2019

Choose a reason for hiding this comment

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

Thank you for looking at this, @iSazonov.

You made several performance-related recommendations:

  1. Use a temporary local in place of the ref int offset in the loop that skips control sequence parameters.
  2. Put str.Length into a temporary local.
  3. I extrapolated from (1) to use a temporary local for offset for a larger portion of the function.

It wasn't clear to me that these changes would improve things. I suspect the results of these changes might be small enought that they would be difficult to measure, so I haven't tried that, but I did look at the disassembly produced by the JIT compiler for an x64 Release version of the binary.

For the first case, there are more instructions and a little more stack and register pressure. Perhaps this would be made up for by eliminating a memory access; however the memory must be accessed eventually anyway, and that memory is on the stack, in the preceding frame--I don't expect it to leave L1 cache. So I don't know if it improves things, but I wouldn't guess it is much.

For the second, it looks like str.Length had already been put into a register, and the only thing the change does is create more stack and register pressure, so I think this could only actually make things worse (though I bet it would be difficult to measure).

For the third, it looks similar to (1), but maybe not quite so bad? But again, it doesn't look like a clear win to me.

You can see the source changes and associated disassembly here.

Copy link
Collaborator

@iSazonov iSazonov Jul 26, 2019

Choose a reason for hiding this comment

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

@jazzdelightsme Thanks!
The method is not call another methods so runtime can optimize the code very efficient.
Nevertheless, we can do something. I propose to make this method local in ControlSequenceLength so that we get direct access to "offset" and "str" variables in ControlSequenceLength.

}
while ((offset < str.Length) && (char.IsDigit(c) || c == ';'));

if (char.IsDigit(c) || c == ';')
continue;
// Finally, handle the command characters for the specific sequences we
// handle:
if (c == 'm')
{
// SGR: Select Graphics Rendition
return offset - start;
}

// Maybe XTPUSHSGR or XTPOPSGR, but we need to read another char. Offset is
// already positioned on the next char (or past the end).
if (offset >= str.Length)
{
return 0;
}

return offset - start;
if (c == '#')
{
// '{' : XTPUSHSGR
// '}' : XTPOPSGR
// 'p' : alias for XTPUSHSGR
// 'q' : alias for XTPOPSGR
c = str[offset++];
if ((c == '{') ||
(c == '}') ||
(c == 'p') ||
(c == 'q'))
{
return offset - start;
}
}

return 0;
}

/// <summary>
Expand Down
21 changes: 20 additions & 1 deletion test/powershell/Host/ConsoleHost.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,8 +795,9 @@ public enum ShowWindowCommands : int
}

Describe "Console host api tests" -Tag CI {
Context "String escape sequences" {
Context "String escape and control sequences" {
$esc = [char]0x1b
$csi = [char]0x9b
$testCases =
@{InputObject = "abc"; Length = 3; Name = "No escapes"},
@{InputObject = "${esc} [31mabc"; Length = 9; Name = "Malformed escape - extra space"},
Expand All @@ -807,11 +808,29 @@ Describe "Console host api tests" -Tag CI {
{
@{InputObject = "$esc[31mabc"; Length = 3; Name = "Escape at start"}
@{InputObject = "$esc[31mabc$esc[0m"; Length = 3; Name = "Escape at start and end"}
@{InputObject = "${csi}31mabc"; Length = 3; Name = "C1 CSI at start"}
@{InputObject = "${csi}31mabc${csi}0m"; Length = 3; Name = "C1 CSI at start and end"}
@{InputObject = "abc${csi}m"; Length = 3; Name = "C1 CSI, no params"}
@{InputObject = "abc${csi}#{"; Length = 3; Name = "C1 CSI, XTPUSHSGR"}
@{InputObject = "abc${csi}#}"; Length = 3; Name = "C1 CSI, XTPOPSGR"}
@{InputObject = "abc${csi}#p"; Length = 3; Name = "C1 CSI, XTPUSHSGR (alias)"}
@{InputObject = "abc${csi}#q"; Length = 3; Name = "C1 CSI, XTPOPSGR (alias)"}
@{InputObject = "abc${esc}[0#p"; Length = 3; Name = "XTPUSHSGR, with param"}
@{InputObject = "${esc}[0;1#qabc"; Length = 3; Name = "XTPOPSGR, with multiple params"}
}
else
{
@{InputObject = "$esc[31mabc"; Length = 8; Name = "Escape at start - no virtual term support"}
@{InputObject = "$esc[31mabc$esc[0m"; Length = 12; Name = "Escape at start and end - no virtual term support"}
@{InputObject = "${csi}31mabc"; Length = 7; Name = "C1 CSI at start - no virtual term support"}
@{InputObject = "${csi}31mabc${csi}0m"; Length = 10; Name = "C1 CSI at start and end - no virtual term support"}
@{InputObject = "abc${csi}m"; Length = 5; Name = "C1 CSI, no params - no virtual term support"}
@{InputObject = "abc${csi}#{"; Length = 6; Name = "C1 CSI, XTPUSHSGR - no virtual term support"}
@{InputObject = "abc${csi}#}"; Length = 6; Name = "C1 CSI, XTPOPSGR - no virtual term support"}
@{InputObject = "abc${csi}#p"; Length = 6; Name = "C1 CSI, XTPUSHSGR (alias) - no virtual term support"}
@{InputObject = "abc${csi}#q"; Length = 6; Name = "C1 CSI, XTPOPSGR (alias) - no virtual term support"}
@{InputObject = "abc${esc}[0#p"; Length = 8; Name = "XTPUSHSGR, with param - no virtual term support"}
@{InputObject = "${esc}[0;1#qabc"; Length = 10; Name = "XTPOPSGR, with multiple params - no virtual term support"}
Copy link
Contributor Author

@jazzdelightsme jazzdelightsme Jul 23, 2019

Choose a reason for hiding this comment

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

@{InputObject = "${esc}[0;1#qabc"; Length = 10; Name = "XTPOPSGR, with multiple params - no virtual term support"} [](start = 12, length = 114)

Oh, actually, I don't know how to run the tests such that SupportsVirtualTerminal is false... is there a convenient way?

Copy link
Contributor

@lzybkr lzybkr Jul 31, 2019

Choose a reason for hiding this comment

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

On Windows 10 - use the "legacy terminal".
NoVirtualTerminal

}

It "Should properly calculate buffer cell width of '<Name>'" -TestCases $testCases {
Expand Down