Skip to content
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

Avoid unnecessary location argument for VB compiling #272

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

angelowang
Copy link

@angelowang angelowang commented Jul 2, 2023

For CompiledExpressionInvoker.ProcessLocationReferences(), the microsoft reference source is not always creating location argument, but only when RequiresCompilation is 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)

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
Copy link
Contributor

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.

@angelowang
Copy link
Author

angelowang commented Aug 1, 2023

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 :)

Copy link
Collaborator

@aoltean16 aoltean16 left a 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

@aoltean16
Copy link
Collaborator

aoltean16 commented Jan 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants