Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ade6b54
PowerShell profiler
iSazonov Sep 22, 2020
cabadd5
Fix CodeFactor issues
iSazonov Sep 22, 2020
49e48f8
Add scriptblock rundown event
iSazonov Sep 23, 2020
84b4629
Use ConditionalWeakTable
iSazonov Sep 23, 2020
f93bd8f
Revert "Use ConditionalWeakTable"
iSazonov Sep 28, 2020
f3e3129
Initialize CompiledScriptBlockTable with scriptblockCache
iSazonov Sep 28, 2020
354a41b
Simplify scriptblock cache
iSazonov Sep 28, 2020
1f17573
Add temporary helpuri
iSazonov Sep 28, 2020
12760c8
Fix tests
iSazonov Sep 28, 2020
a9ae2a2
Add runspace and parent scriptblock Ids in perf event
iSazonov Sep 29, 2020
7c631e4
Add Start Opcode
iSazonov Sep 29, 2020
f8ef7a4
Add ScriptBlockId to output
iSazonov Sep 29, 2020
dc344ef
Get correct parent id
iSazonov Sep 29, 2020
2f84f19
Add tracking default parameter expressions
iSazonov Sep 30, 2020
03ff234
Avoid lock if scriptblock is already compiled
iSazonov Sep 30, 2020
a7d6abb
Add support runspaces to Measure-ScriptBlock
iSazonov Oct 13, 2020
8899bb6
Address Robert's feedback
iSazonov Feb 16, 2021
483b2ea
Make file parameter nullable
iSazonov Mar 24, 2021
9bdc763
Fix style issues
iSazonov Mar 24, 2021
4867a45
Fix style issues 2
iSazonov Mar 25, 2021
3d260ba
Experiment with UpdatePosition()
iSazonov Mar 25, 2021
04bcd4c
Revert "Experiment with UpdatePosition()"
iSazonov Mar 26, 2021
ffdfc91
Add profiling in EnterScriptFunction()
iSazonov Mar 26, 2021
83b72d3
Move logging to UpdatePosition()
iSazonov Mar 29, 2021
bf38033
add temp URI to get it to build
SteveL-MSFT May 23, 2022
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
13 changes: 10 additions & 3 deletions src/System.Management.Automation/engine/ExternalScriptInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,10 @@ public override SessionStateEntryVisibility Visibility
{
get
{
if (Context == null) return SessionStateEntryVisibility.Public;
if (Context == null)
{
return SessionStateEntryVisibility.Public;
}

return Context.EngineSessionState.CheckScriptVisibility(_path);
}
Expand Down Expand Up @@ -267,7 +270,11 @@ internal ScriptBlockAst GetScriptBlockAst()
var scriptContents = ScriptContents;
if (_scriptBlock == null)
{
this.ScriptBlock = ScriptBlock.TryGetCachedScriptBlock(_path, scriptContents);
CompiledScriptBlockData compiledScriptBlockData = ScriptBlock.TryGetCachedCompiledScriptBlock(_path, scriptContents);
if (compiledScriptBlockData != null)
{
this.ScriptBlock = new ScriptBlock(compiledScriptBlockData);
}
Comment on lines +273 to +277
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid making changes to the call sites of TryGetCachedScriptBlock. Whether it caches ScriptBlock or CompiledScriptBlockData should be an implementation detail to TryGetCachedScriptBlock.

}

if (_scriptBlock != null)
Expand Down Expand Up @@ -305,7 +312,7 @@ internal ScriptBlockAst GetScriptBlockAst()
if (errors.Length == 0)
{
this.ScriptBlock = new ScriptBlock(_scriptBlockAst, isFilter: false);
ScriptBlock.CacheScriptBlock(_scriptBlock.Clone(), _path, scriptContents);
ScriptBlock.CacheCompiledScriptBlock(_scriptBlock.Clone(), _path, scriptContents);
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5269,6 +5269,7 @@ private static void InitializeCoreCmdletsAndProviders(
{ "Import-Module", new SessionStateCmdletEntry("Import-Module", typeof(ImportModuleCommand), helpFile) },
{ "Invoke-Command", new SessionStateCmdletEntry("Invoke-Command", typeof(InvokeCommandCommand), helpFile) },
{ "Invoke-History", new SessionStateCmdletEntry("Invoke-History", typeof(InvokeHistoryCommand), helpFile) },
{ "Measure-Script", new SessionStateCmdletEntry("Measure-Script", typeof(MeasureScriptCommand), helpFile) },
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jun 13, 2022

Choose a reason for hiding this comment

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

Trace-Script seems like a better name for what it does currently. A separate Measure-Script may be needed later that leverages Trace-Script, but produces summarized output to the user. I don't think Measure-Script needs to be part of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Measure-Script is the name of the main cmdlet in PSProfiler. Trace-Script is the name of the main cmdlet in my Profiler module, but only because Measure-Script is already taken. Both these cmdlets provide a summary.

So using Trace-Script here to get the "raw" data, is imho the best choice. And I am okay with renaming mine.

{ "New-Module", new SessionStateCmdletEntry("New-Module", typeof(NewModuleCommand), helpFile) },
{ "New-ModuleManifest", new SessionStateCmdletEntry("New-ModuleManifest", typeof(NewModuleManifestCommand), helpFile) },
{ "New-PSRoleCapabilityFile", new SessionStateCmdletEntry("New-PSRoleCapabilityFile", typeof(NewPSRoleCapabilityFileCommand), helpFile) },
Expand Down
29 changes: 29 additions & 0 deletions src/System.Management.Automation/engine/debugger/debugger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,16 @@ internal void EnterScriptFunction(FunctionContext functionContext)
Diagnostics.Assert(functionContext._executionContext == _context, "Wrong debugger is being used.");

var invocationInfo = (InvocationInfo)functionContext._localsTuple.GetAutomaticVariable(AutomaticVariable.MyInvocation);

if (ProfilerEventSource.LogInstance.IsEnabled())
{
ProfilerEventSource.LogInstance.SequencePoint(
functionContext._scriptBlock?.Id ?? Guid.Empty,
functionContext._executionContext.CurrentRunspace.InstanceId,
_callStack.Last()?.FunctionContext._scriptBlock.Id ?? Guid.Empty,
0);
}

var newCallStackInfo = new CallStackInfo
{
InvocationInfo = invocationInfo,
Expand Down Expand Up @@ -1232,6 +1242,25 @@ internal void RegisterScriptFile(string path, string scriptContents)
}
}

internal Guid GetParentScriptBlockId(int sequencePointPosition)
{
// Sequence point on 0 position is an entry point in a scriptblock.
// In the time the callstack has still point to parent scriptblock
// so we take last element from the callstack,
// for rest sequence points we take an element before last as parent.
Comment on lines +1247 to +1250
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't fully understand this comment. Would you be able to describe the algorithm you're implementing here and we can put together a comment around that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to handle the condition when we go into a nested scriptblock. If an sequence point on 0 position its parent scriptblock on stack is in position -1 but for follow sequence points we already has current scriptblock in the stack and a shift to the parent is -2.

Copy link
Member

Choose a reason for hiding this comment

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

I think the comment needs to be reworded to make it more understandable. I wonder if an example scriptblock in the comment would help.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please reword the comments here? The algorithm is not clear to me by reading the existing comments. An example would be helpful too.

int shift = sequencePointPosition == 0 ? 1 : 2;
if (_callStack.Count - shift >= 0)
{
ScriptBlock scriptBlock = _callStack[_callStack.Count - shift].FunctionContext._scriptBlock;
if (scriptBlock is not null)
{
return scriptBlock.Id;
}
}

return Guid.Empty;
}

#endregion Call stack management

#region setting breakpoints
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,13 @@ private UpdatePositionInstruction(bool checkBreakpoints, int sequencePoint)
public override int Run(InterpretedFrame frame)
{
var functionContext = frame.FunctionContext;
var context = frame.ExecutionContext;

functionContext._currentSequencePointIndex = _sequencePoint;
if (_checkBreakpoints)
{
if (context._debuggingMode > 0)
{
context.Debugger.OnSequencePointHit(functionContext);
}
functionContext.UpdatePosition(_sequencePoint);
}
else
{
functionContext.UpdatePositionNoBreak(_sequencePoint);
}
Comment on lines -32 to 39
Copy link
Member

Choose a reason for hiding this comment

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

Should be changed to

var functionContext = frame.FunctionContext;
functionContext._currentSequencePointIndex = _sequencePoint;

if (_checkBreakpoints)
{
    functionContext.UpdatePosition(_sequencePoint);
}


return +1;
Expand Down
71 changes: 50 additions & 21 deletions src/System.Management.Automation/engine/parser/Compiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,12 @@ internal static class CachedReflectionInfo
internal static readonly MethodInfo FunctionContext_PushTrapHandlers =
typeof(FunctionContext).GetMethod(nameof(FunctionContext.PushTrapHandlers), InstanceFlags);

internal static readonly MethodInfo FunctionContext_UpdatePosition =
typeof(FunctionContext).GetMethod(nameof(FunctionContext.UpdatePosition), InstanceFlags);

internal static readonly MethodInfo FunctionContext_UpdatePositionNoBreak =
typeof(FunctionContext).GetMethod(nameof(FunctionContext.UpdatePositionNoBreak), InstanceFlags);

internal static readonly MethodInfo FunctionOps_DefineFunction =
typeof(FunctionOps).GetMethod(nameof(FunctionOps.DefineFunction), StaticFlags);

Expand Down Expand Up @@ -802,6 +808,30 @@ internal void PopTrapHandlers()
{
_traps.RemoveAt(_traps.Count - 1);
}

internal void UpdatePositionNoBreak(int pos)
{
_currentSequencePointIndex = pos;
}
Copy link
Member

Choose a reason for hiding this comment

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

So, we only emit events when _checkBreakpoints == true? Why not when it's false, and in what cases _checkBreakpoints would be false?

Also, then this method doesn't really do anything and should be removed.


internal void UpdatePosition(int pos)
{
UpdatePositionNoBreak(pos);

if (ProfilerEventSource.LogInstance.IsEnabled())
{
ProfilerEventSource.LogInstance.SequencePoint(
_scriptBlock?.Id ?? Guid.Empty,
_executionContext.CurrentRunspace.InstanceId,
_executionContext.Debugger.GetParentScriptBlockId(pos),
pos);
}

if (_executionContext._debuggingMode > 0)
{
_executionContext.Debugger.OnSequencePointHit(this);
}
}
Comment on lines +817 to +834
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
internal void UpdatePosition(int pos)
{
UpdatePositionNoBreak(pos);
if (ProfilerEventSource.LogInstance.IsEnabled())
{
ProfilerEventSource.LogInstance.SequencePoint(
_scriptBlock?.Id ?? Guid.Empty,
_executionContext.CurrentRunspace.InstanceId,
_executionContext.Debugger.GetParentScriptBlockId(pos),
pos);
}
if (_executionContext._debuggingMode > 0)
{
_executionContext.Debugger.OnSequencePointHit(this);
}
}
internal void UpdatePosition()
{
if (ProfilerEventSource.LogInstance.IsEnabled())
{
ProfilerEventSource.LogInstance.SequencePoint(
_scriptBlock?.Id ?? Guid.Empty,
_executionContext.CurrentRunspace.InstanceId,
_executionContext.Debugger.GetParentScriptBlockId(_currentSequencePointIndex),
_currentSequencePointIndex);
}
if (_executionContext._debuggingMode > 0)
{
_executionContext.Debugger.OnSequencePointHit(this);
}
}

}

internal class Compiler : ICustomAstVisitor2
Expand Down Expand Up @@ -2145,13 +2175,28 @@ private static object GetExpressionValue(
var pipe = new Pipe(resultList);
try
{
ScriptBlock scriptBlock = null;
if (ProfilerEventSource.LogInstance.IsEnabled())
{
// We need a scriptblock Id to keep track of the call stack.
scriptBlock = ScriptBlock.Create(context, expressionAst.Extent.Text);

// No need to optimize - we only want to add to the scriptblock cache
// so that we can get rundown events.
// We can safely do this because lambda and sequencePoints
// parameters of the method come always from callsites as null.
// (We could remove them at all.)
scriptBlock.Compile(optimized: false);
Copy link
Member

Choose a reason for hiding this comment

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

Is this for getting the script call stack when generating events? It looks quite hacky.

I think we need to define the goal for this PR. I guess the goal is to make the Pester's coverage functionality able to work with the ETW events generated from the changes here. So, is the script stack necessary for the Pester's coverage functionality to work?

If we set that as the goal, we should only include features/changes that are necessary for that to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both Pester code coverage, and Profiler would depend on this. Pester code coverage is a subset of Profiler functionality. Both use this interface to provide their services:

https://github.com/nohwnd/Profiler/blob/main/csharp/Profiler/ITracer.cs

Which collects:

// The currently executing scriptblock and extent
IScriptExtent extent,
ScriptBlock scriptBlock,

// how many items are on callstack
int level,

// the current function and module name
string functionName,
string moduleName

The data is collected here, by reflecting on debugger, functionContext and session state:

https://github.com/nohwnd/Profiler/blob/main/csharp/Profiler/Tracer.cs#L157-L171

As long as this info is provided in some form, the current version of Profiler can work with this.

"Level" I should be able to get from scriptblock ids as pointed out here: #13673 (comment)

Module I should be able to get from the scriptblock and its associated session state. I think.

But what about the function name? Any suggestions where I can grab that?

}

var functionContext = new FunctionContext
{
_sequencePoints = sequencePoints,
_executionContext = context,
_file = expressionAst.Extent.File,
_outputPipe = pipe,
_localsTuple = MutableTuple.MakeTuple(localsTupleType, DottedLocalsNameIndexMap)
_localsTuple = MutableTuple.MakeTuple(localsTupleType, DottedLocalsNameIndexMap),
_scriptBlock = scriptBlock
};
if (usingValues != null)
{
Expand Down Expand Up @@ -7055,26 +7100,10 @@ public override Expression Reduce()
exprs.Add(Expression.DebugInfo(_debugSymbolDocument, _extent.StartLineNumber, _extent.StartColumnNumber, _extent.EndLineNumber, _extent.EndColumnNumber));
}

exprs.Add(
Expression.Assign(
Expression.Field(Compiler.s_functionContext, CachedReflectionInfo.FunctionContext__currentSequencePointIndex),
ExpressionCache.Constant(_sequencePoint)));

if (_checkBreakpoints)
{
exprs.Add(
Expression.IfThen(
Expression.GreaterThan(
Expression.Field(Compiler.s_executionContextParameter, CachedReflectionInfo.ExecutionContext_DebuggingMode),
ExpressionCache.Constant(0)),
Expression.Call(
Expression.Field(Compiler.s_executionContextParameter, CachedReflectionInfo.ExecutionContext_Debugger),
CachedReflectionInfo.Debugger_OnSequencePointHit,
Compiler.s_functionContext)));
}

exprs.Add(ExpressionCache.Empty);

MethodInfo method = _checkBreakpoints
? CachedReflectionInfo.FunctionContext_UpdatePosition
: CachedReflectionInfo.FunctionContext_UpdatePositionNoBreak;
exprs.Add(Expression.Call(Compiler.s_functionContext, method, ExpressionCache.Constant(_sequencePoint)));
Comment on lines +7103 to +7106
Copy link
Member

Choose a reason for hiding this comment

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

Should be changed to:

            exprs.Add(
                Expression.Assign(
                    Expression.Field(Compiler.s_functionContext, CachedReflectionInfo.FunctionContext__currentSequencePointIndex),
                    ExpressionCache.Constant(_sequencePoint)));

            if (_checkBreakpoints)
            {
                exprs.Add(Expression.Call(Compiler.s_functionContext, CachedReflectionInfo.FunctionContext_UpdatePosition);
            }

            exprs.Add(ExpressionCache.Empty);

return Expression.Block(exprs);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,46 @@ internal enum ScriptBlockClauseToInvoke

internal class CompiledScriptBlockData
{
private static readonly ConcurrentDictionary<CompiledScriptBlockData, CompiledScriptBlockData> s_compiledScriptBlockTable
= new ConcurrentDictionary<CompiledScriptBlockData, CompiledScriptBlockData>();
Copy link
Member

Choose a reason for hiding this comment

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

Why do the key and value are the same object? Comments are needed here to explain the intent.


internal static void ClearCompiledScriptBlockTable()
{
s_compiledScriptBlockTable.Clear();
}

internal static ICollection<CompiledScriptBlockData> GetCompiledScriptBlockList()
{
return s_compiledScriptBlockTable.Values;
}

internal static void InitCompiledScriptBlockTable()
{
ClearCompiledScriptBlockTable();

foreach (CompiledScriptBlockData csb in ScriptBlock.GetCachedCompiledScriptBlockData())
{
s_compiledScriptBlockTable.TryAdd(csb, csb);
}
}

internal void RegisterCompiledScriptBlockData()
{
s_compiledScriptBlockTable.TryAdd(this, this);
}

internal CompiledScriptBlockData(IParameterMetadataProvider ast, bool isFilter)
{
_ast = ast;
this.IsFilter = isFilter;
this.Id = Guid.NewGuid();
IsFilter = isFilter;
Id = Guid.NewGuid();
}

internal CompiledScriptBlockData(string scriptText, bool isProductCode)
{
_isProductCode = isProductCode;
_scriptText = scriptText;
this.Id = Guid.NewGuid();
Id = Guid.NewGuid();
}

internal bool Compile(bool optimized)
Expand Down Expand Up @@ -78,6 +106,11 @@ internal bool Compile(bool optimized)
CompileOptimized();
}

if (ProfilerEventSource.LogInstance.IsEnabled())
{
RegisterCompiledScriptBlockData();
}

return optimized;
}

Expand Down Expand Up @@ -131,6 +164,11 @@ private void InitializeMetadata()

private void CompileUnoptimized()
{
if (_compiledUnoptimized)
{
return;
}

lock (this)
{
if (_compiledUnoptimized)
Expand All @@ -146,6 +184,11 @@ private void CompileUnoptimized()

private void CompileOptimized()
{
if (_compiledOptimized)
{
return;
}

lock (this)
{
if (_compiledOptimized)
Expand Down Expand Up @@ -548,7 +591,7 @@ internal ScriptBlock(IParameterMetadataProvider ast, bool isFilter)
{
}

private ScriptBlock(CompiledScriptBlockData scriptBlockData)
internal ScriptBlock(CompiledScriptBlockData scriptBlockData)
{
_scriptBlockData = scriptBlockData;

Expand Down Expand Up @@ -576,24 +619,25 @@ protected ScriptBlock(SerializationInfo info, StreamingContext context)
{
}

private static readonly ConcurrentDictionary<Tuple<string, string>, ScriptBlock> s_cachedScripts =
new ConcurrentDictionary<Tuple<string, string>, ScriptBlock>();
private static readonly ConcurrentDictionary<Tuple<string, string>, CompiledScriptBlockData> s_cachedScripts =
new ConcurrentDictionary<Tuple<string, string>, CompiledScriptBlockData>();

internal static ICollection<CompiledScriptBlockData> GetCachedCompiledScriptBlockData()
{
return s_cachedScripts.Values;
}

internal static ScriptBlock TryGetCachedScriptBlock(string fileName, string fileContents)
internal static CompiledScriptBlockData TryGetCachedCompiledScriptBlock(string fileName, string fileContents)
Copy link
Member

Choose a reason for hiding this comment

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

How about keeping the method signature unchanged, but only changing the cache to hold CompiledScriptBlockData instead? In that way, the call sites of the TryGetCacheXXX and CacheXXX methods are unchanged. It's really just an implementation detail of those 2 methods.

{
if (InternalTestHooks.IgnoreScriptBlockCache)
{
return null;
}

ScriptBlock scriptBlock;
var key = Tuple.Create(fileName, fileContents);
if (s_cachedScripts.TryGetValue(key, out scriptBlock))
if (s_cachedScripts.TryGetValue(key, out var compiledScriptBlockData))
{
Diagnostics.Assert(
scriptBlock.SessionStateInternal == null,
"A cached scriptblock should not have it's session state bound, that causes a memory leak.");
return scriptBlock.Clone();
return compiledScriptBlockData;
}

return null;
Expand All @@ -605,7 +649,7 @@ private static bool IsDynamicKeyword(Ast ast)
private static bool IsUsingTypes(Ast ast)
=> ast is UsingStatementAst cmdAst && cmdAst.IsUsingModuleOrAssembly();

internal static void CacheScriptBlock(ScriptBlock scriptBlock, string fileName, string fileContents)
internal static void CacheCompiledScriptBlock(ScriptBlock scriptBlock, string fileName, string fileContents)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, better make the signature unchanged. What to actually cache is an implementation detail.

{
if (InternalTestHooks.IgnoreScriptBlockCache)
{
Expand All @@ -632,26 +676,18 @@ internal static void CacheScriptBlock(ScriptBlock scriptBlock, string fileName,
}

var key = Tuple.Create(fileName, fileContents);
s_cachedScripts.TryAdd(key, scriptBlock);
}

/// <summary>
/// Clears the cached scriptblocks.
/// </summary>
internal static void ClearScriptBlockCache()
{
s_cachedScripts.Clear();
s_cachedScripts.TryAdd(key, scriptBlock.ScriptBlockData);
}

internal static readonly ScriptBlock EmptyScriptBlock =
ScriptBlock.CreateDelayParsedScriptBlock(string.Empty, isProductCode: true);

internal static ScriptBlock Create(Parser parser, string fileName, string fileContents)
{
var scriptBlock = TryGetCachedScriptBlock(fileName, fileContents);
if (scriptBlock != null)
var compiledScriptBlockData = TryGetCachedCompiledScriptBlock(fileName, fileContents);
if (compiledScriptBlockData != null)
{
return scriptBlock;
return new ScriptBlock(compiledScriptBlockData);
}

var ast = parser.Parse(fileName, fileContents, null, out ParseError[] errors, ParseMode.Default);
Expand All @@ -661,12 +697,9 @@ internal static ScriptBlock Create(Parser parser, string fileName, string fileCo
}

var result = new ScriptBlock(ast, isFilter: false);
CacheScriptBlock(result, fileName, fileContents);
CacheCompiledScriptBlock(result, fileName, fileContents);

// The value returned will potentially be bound to a session state. We don't want
// the cached script block to end up being bound to any session state, so clone
// the return value to ensure the cached value has no session state.
return result.Clone();
return result;
}

internal ScriptBlock Clone() => new ScriptBlock(_scriptBlockData);
Expand Down
Loading