-
Notifications
You must be signed in to change notification settings - Fork 231
Avoid unnecessary location argument for VB compiling#272
Avoid unnecessary location argument for VB compiling #272angelowang wants to merge 1 commit intoUiPath:develop from
Conversation
For CompiledExpressionInvoker.ProcessLocationReferences, the microsoft reference source is not always create location argument, but only when `RequiresCompilation` is true, which is for C#. This helps for performance a lot when there are a lot of expressions to compile.
dmetzgar
commented
Jul 31, 2023
Reference source is for the .NET Framework. In .NET Framework WF, VB expressions are compiled using a native VB expression compiler at runtime. C# expressions have to be compiled with csc through MsBuild so are compiled at build time. CoreWF uses Roslyn, which is the same for both VB and C#. The whole "requirescompilation" flag then becomes obsolete and is only kept for back-compat. Either both VB and C# should have this change or they both should not.
Framework: VB - Host Compiler - Runtime compiling - _accessor.CreateLocationArgument(reference, false); not run.
So if I am understanding correctly, in NET6, if they are the same, they both should not call that line? But we still have the check for this:
if (_textExpression.Language == "VB")
{
IList<string> requiredLocationNames = _compiledRoot.GetRequiredLocations(_expressionId);
CreateRequiredArguments(requiredLocationNames);
}
Seems already inconsistent.
BTW, CSharpValue code are still similar to .NET framework, and Execute() is using _invoker, instead of some cached _expressionTree.Compile(). That's why I understood they are still different and the requires compilation flag is still somewhat being used.
Our product (using only VB script) removes that and improves performance quite a lot. CoreWF unit tests passing, and our test results also look good :)
@aoltean16
aoltean16
left a comment
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.
For netcore, RequiresCompilation is always set to true. (see ActivityXamlServices.cs line 585)
Considering this, I dont think the PR is relevant anymore.
Thanks
Framework: VB - Host Compiler - Runtime compiling -
_accessor.CreateLocationArgument(reference, false);not run.So if I am understanding correctly, in NET6, if they are the same, they both should not call that line? But we still have the check for this:
if (_textExpression.Language == "VB") { IList<string> requiredLocationNames = _compiledRoot.GetRequiredLocations(_expressionId); CreateRequiredArguments(requiredLocationNames); }Seems already inconsistent.
BTW, CSharpValue code are still similar to .NET framework, and Execute() is using _invoker, instead of some cached _expressionTree.Compile(). That's why I understood they are still different and the requires compilation flag is still somewhat being used.
Our product (using only VB script) removes that and improves performance quite a lot. CoreWF unit tests passing, and our test results also look good :)
I will have a look at it.
Uh oh!
There was an error while loading. Please reload this page.
For
CompiledExpressionInvoker.ProcessLocationReferences(), the microsoft reference source is not always creating location argument, but only whenRequiresCompilationis true, which is for C#.https://github.com/microsoft/referencesource/blob/master/System.Activities/System/Activities/Expressions/CompiledExpressionInvoker.cs#L245
This helps for performance a lot when there are a lot of expressions to compile.
(21 complex xaml files, the first Execute() drops from 27s to 20s)