Skip to content

WIP: Marshal Script-based ToString to PS Execution Pipeline for Performance #1688

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

Closed
wants to merge 1 commit into from
Closed

WIP: Marshal Script-based ToString to PS Execution Pipeline for Performance #1688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 25 additions & 1 deletion src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Linq;
using System.Management.Automation;
using System.Management.Automation.Language;
using System.Management.Automation.Runspaces;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -263,7 +264,30 @@ public VariableDetailsBase[] GetVariables(int variableReferenceId)
VariableDetailsBase parentVariable = variables[variableReferenceId];
if (parentVariable.IsExpandable)
{
childVariables = parentVariable.GetChildren(_logger);
// PERFORMANCE: Because of ToString calls that might be PS script properties, we want this
// to run on the pipeline thread to avoid retry timeouts
// TODO: Cancellation Token Maybe?
// TODO: Maybe pass
childVariables = _psesHost.InvokeDelegate<VariableDetailsBase[]>
(
representation: "GetChildren",
ExecutionOptions.Default,
(_) =>
{
var existingRunspace = Runspace.DefaultRunspace;
Copy link
Member

Choose a reason for hiding this comment

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

I'm just curious why you need to save, change, and then reset the runspace?

Copy link
Collaborator Author

@JustinGrote JustinGrote Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@SeeminglyScience can comment but I think it's just a safety/idempotency thing, in all likelihood it's the same runspace so this effectively doesn't do anything.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, if that's something we need to do perhaps we should do it in a wrapper? Like, where/when else might we have to do something like that?

And if we're not sure that we have to do it, I'd rather leave a TODO for now so if we encounter a bug we fix it later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that was a troubleshooting step that didn't do anything right? If so that can be removed for sure.

try
{
Runspace.DefaultRunspace = _psesHost.CurrentRunspace.Runspace;
return parentVariable.GetChildren(_logger);
}
finally
{
Runspace.DefaultRunspace = existingRunspace;
}
},
CancellationToken.None
);

foreach (var child in childVariables)
{
// Only add child if it hasn't already been added.
Expand Down
18 changes: 18 additions & 0 deletions test/PowerShellEditorServices.Test.Shared/Debugging/VariableTest.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,21 @@ $sortedDictionary['blue'] = 'red'

# This is a dummy function that the test will use to stop and evaluate the debug environment
function __BreakDebuggerDerivedDictionaryPropertyInRawView{}; __BreakDebuggerDerivedDictionaryPropertyInRawView


class CustomToString {
[String]$String = 'Hello'
[String]ToString() {
return $this.String.ToUpper()
}
}
$SCRIPT:CustomToStrings = 1..1000 | ForEach-Object {
[CustomToString]::new()
}
$SCRIPT:Small = $SCRIPT:CustomToStrings[1..10]
$SCRIPT:Medium = $SCRIPT:CustomToStrings[1..50]
$SCRIPT:Large = $SCRIPT:CustomToStrings[1..100]


# This is a dummy function that the test will use to stop and evaluate the debug environment
function __BreakDebuggerToStringShouldMarshallToPipeline{}; __BreakDebuggerToStringShouldMarshallToPipeline
20 changes: 20 additions & 0 deletions test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,26 @@ await debugService.SetLineBreakpointsAsync(
Assert.Equal("73", childVars[1].ValueString);
}

/// <summary>
/// Verifies Issue #1686
/// </summary>
[Fact]
public async Task DebuggerToStringShouldMarshallToPipeline()
{
CommandBreakpointDetails breakpoint = CommandBreakpointDetails.Create("__BreakDebuggerToStringShouldMarshallToPipeline");
await debugService.SetCommandBreakpointsAsync(new[] { breakpoint }).ConfigureAwait(true);

// Execute the script and wait for the breakpoint to be hit
Task _ = ExecuteVariableScriptFile();
AssertDebuggerStopped(commandBreakpointDetails: breakpoint);

VariableDetailsBase variableArray = Array.Find(
GetVariables(VariableContainerDetails.ScriptScopeName),
v => v.Name == "$CustomToStrings"
);
Assert.NotNull(variableArray);
}

// Verifies fix for issue #86, $proc = Get-Process foo displays just the ETS property set
// and not all process properties.
[Fact(Skip = "Length of child vars is wrong now")]
Expand Down