-
Notifications
You must be signed in to change notification settings - Fork 635
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.
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
JIL Execution Cleanup and Speed Optimizations Part 1 #12895
JIL Execution Cleanup and Speed Optimizations Part 1 #12895
Conversation
@jasonstratton @aparajit-pratap Closing #12153 in favor of this. This removes the commit refactoring |
@@ -1422,7 +1428,6 @@ private int UpdateGraph(int exprUID, bool isSSAAssign) | |||
} | |||
gnode.isActive = false; | |||
} | |||
return reachableGraphNodes.Count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we keep this return
statement, that way we can just say return GraphUpdateImpl(...);
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that makes sence. I will update that.
{ | ||
frame[destIndex] = Stack[sourceIndex]; | ||
} | ||
var stackFrame = new StackFrame(frame); | ||
|
||
stackFrame = new StackFrame(frame); | ||
return stackFrame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a redundant return
as you're returning the stackFrame
outside the if
block anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep good catch
get | ||
{ | ||
if (FramePointer - StackFrame.StackFrameSize >= startFramePointer) | ||
{ | ||
return null; | ||
return false; | ||
} | ||
|
||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we simply say:
get { stackFrame == null; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to initialize the stackFrame
object unless you actually need a StackFrame
object. That is the big time penalty here. All of these essentially copy the implementation from StackFrame here to ovoid the getter for the CurrentStackFrame. On my sample graph it is called that getter is called 21 million times.
@@ -294,32 +294,101 @@ public int CurrentConstructBlockId | |||
} | |||
} | |||
|
|||
|
|||
private int fp; | |||
private StackFrame stackFrame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to currentStackFrame
Purpose
JIL Execution in the VM is responsible for a small but important subset of node execution. While most nodes today are executed via the FFI (Foreign Function Interpreter) mechanisms which invoke C# code, some basic utility methods especially in Math category are written in native design script. This type of function is handled by the
JILFunctionEndPoint
. While many of these enhancements are targeted at this execution path, they also yield net improvements to the general execution of all EndPoint types. This is especially true in thePOP_Handler
,DEP_Handler
,SetupExecutive
, andRestoreFromCall
methods within the VM's Executive. For a specific test graph with a mixture of geometry and mathematical operations, these enhancements specifically reduce overallUpdateGraph
run time by 35% (17s to 13s). The net impact is also dramatically reduces temp memory allocation. For the case of this specific test graph the temporary memory allocation associated withUpdateGraph
went from 11.3gb to 3.6gb. In summary this PR optimizes the execution of functions handled via the JIL Endpoint but has a net improvement to all node types.Specifically this PR does the following
Clean up dependency on
CurrentStackFrame
property ofRuntimeMemory
. This getter creates a newStackFrame
object to reference a subset of items at a specific location in the VM's Stack. TheCurentStackFrame
property is utilized 99% of the time from theIsGlobalScope
method. This issue isIsGlobalScope
can be called tens of millions of times during a Graph Execution run which creates a newStackFrame
object every time theCurrentStackFrame
property is referenced. This optimization simply removes the need to allocate a temporaryStackValue
object when the data which is needed can be easily referenced directly from the Stack. This optimization represents the majority of the extra gigabytes of temporary allocation described above in the sample graph performance delta.Refactor
RestoreFromCall
to not allocate empty list until after the required check ofruntimeCore.Options.RunMode == InterpreterMode.Expression
. This allocation is done repeatedly with no items added to the collection. The call later to check the list via Any() checks can be refactored to a null check.Fast path for
GetGraphNodesAtScope
when asking for a Invalid ClassIndex and ProcessIndex (ie -1). This is another function that can be called millions of times during a UpdateGraph run. Many calls that are routed through this function are looking for the same item in thegraphNodeMap
dictionary. This optimization creates a shortcut when the lookup is asking for the specific case of the invalidClassIndex
andProcessIndex
that avoids accessing the object from the dictionary. Note, in the case of this function, caching the previous lookup would not speed up the lookup as the method usage typically alternates between values.Refactor
UpdateGraph
to not allocate a temporary list of GraphNodes.Declarations
Check these if you believe they are true
*.resx
filesReviewers
@sm6srw @aparajit-pratap
FYIs
@jasonstratton @mjkkirschner