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.

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

Jump to bottom

[Source Gen] Generate function executor code #1309

Merged
merged 20 commits into from
Mar 22, 2023
Merged

[Source Gen] Generate function executor code #1309

merged 20 commits into from
Mar 22, 2023

Conversation

kshyju
Copy link
Member

@kshyju kshyju commented Feb 3, 2023 •
edited
Loading

Fixes #1284

This source generator creates an implementation of IFunctionExecutor. Our current implementation of IFunctionExecutor relies on reflection at runtime (so it has an impact on coldstart). We have tried a hard coded version in our coldstart test environment and it proved to improve cold start numbers since no reflection code is being executed. In this PR, the source gen creates an implementation which is similar to the one linked above. We rely on IFunctionActivator in this version to create an instance of the function class.

Generator needs to be enabled explicitly by using <FunctionsEnableExecutorSourceGen>True</FunctionsEnableExecutorSourceGen> in project file (of the function app)

@kshyju
Copy link
Member Author

kshyju commented Feb 3, 2023

We still need to discuss how to register this implementation to the DI container automatically during bootstrapping so that customer does not need to explicitly register it. Same for our src gen version of function metadata provider.

https://github.com/kshyju/FunctionApp26/blob/666b0af271a66f2c8674a76ad6a0e6a798f7cd73/FunctionApp26/Program.cs#L9-L13

@satvu
Copy link
Member

satvu commented Feb 3, 2023

Wanted to add that I'm a big fan of the refactoring 😍 taking out the SyntaxReceiver and also adding FunctionsUtil is going to be so helpful for future work!

kshyju
kshyju commented Feb 3, 2023
View reviewed changes
sdk/Sdk.Generators/Constants.cs
@@ -30,9 +32,6 @@ public static class Types
// System types
internal const string IEnumerable = "System.Collections.IEnumerable";
internal const string IEnumerableGeneric = "System.Collections.Generic.IEnumerable`1";
internal const string IEnumerableOfString = "System.Collections.Generic.IEnumerable`1<System.String>";
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed unused.

@kshyju kshyju requested a review from satvu February 3, 2023 21:12
@kshyju kshyju force-pushed the shkr/1284_srcgen-default-executor branch from 2529c6b to 3969a98 Compare February 6, 2023 16:40
@kshyju kshyju force-pushed the shkr/1284_srcgen-default-executor branch from 3969a98 to 7bd807d Compare February 8, 2023 20:42
@kshyju kshyju force-pushed the shkr/1284_srcgen-default-executor branch from 5a34157 to 5331e3a Compare February 10, 2023 19:47
@kshyju kshyju mentioned this pull request Feb 14, 2023
Merged
@kshyju kshyju force-pushed the shkr/1284_srcgen-default-executor branch 2 times, most recently from a9cf275 to 7efa437 Compare February 23, 2023 22:51
@kshyju
Copy link
Member Author

kshyju commented Feb 24, 2023

Ping for review @fabiocav @brettsam

@kshyju kshyju force-pushed the shkr/1284_srcgen-default-executor branch from 4c22ea8 to 748dfea Compare February 24, 2023 21:00
fabiocav
fabiocav reviewed Mar 4, 2023
View reviewed changes
Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

Initial (partial) review

sdk/Sdk.Generators/FunctionExecutor/FunctionExecutorGenerator.Emitter.cs Outdated
private static string GetMethodBody(IEnumerable<ExecutableFunction> functions)
{
var sb = new StringBuilder();
sb.Append(@"var inputBindingFeature = context.Features.Get<IFunctionInputBindingFeature>()!;
Copy link
Member

Choose a reason for hiding this comment

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

nit: raw string literals would improve readability here

sdk/Sdk.Generators/FunctionExecutor/FunctionExecutorGenerator.Emitter.cs Outdated
");
foreach (ExecutableFunction function in functions)
{
sb.Append($@"
Copy link
Member

Choose a reason for hiding this comment

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

To improve readability, you can build the arguments then craft the entire string with those arguments injected. That will remove the number of string fragments you have here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched and I feel the readability is gone. The indentation was driving me crazy. I ended up adjusting code here to match with the expected output code in tests. I am sure there is a better way to write this. Yea, I don't like the current version.

sdk/Sdk.Generators/FunctionExecutor/FunctionExecutorGenerator.Parser.cs Outdated
private readonly GeneratorExecutionContext _context;
private readonly KnownTypes _knownTypes;

private Compilation Compilation => _context.Compilation;
Copy link
Member

Choose a reason for hiding this comment

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

nit: move below constructor.

sdk/Sdk.Generators/FunctionsUtil.cs Outdated
/// <summary>
/// Checks if a candidate method has a Function attribute on it.
/// </summary>
internal static bool IsValidMethodAzureFunction(
Copy link
Member

Choose a reason for hiding this comment

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

nit: naming -> IsValidFunctionMethod or IsFunctionMethod

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed.

sdk/Sdk/Targets/Microsoft.Azure.Functions.Worker.Sdk.props Outdated
@@ -22,6 +22,7 @@ WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and

<ItemGroup>
<CompilerVisibleProperty Include="FunctionsEnableMetadataSourceGen" />
<CompilerVisibleProperty Include="FunctionsEnablePlaceholder" />
Copy link
Member

Choose a reason for hiding this comment

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

Since the generator is not related to placeholders, I think we need a better name here. Should also be something that clearly describes that it controls the source generation enablement. Perhaps FunctionsEnableExecutorSourceGenerator, though, something shorter would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched to "FunctionsEnableExecutorSourceGen". We are using "FunctionsEnablePlaceholder" to control behavior such as "not generating the extension entry(of legacy json metadata generator) in function.json". Let's sync later whether to consolidate that with something else.

sdk/Sdk.Generators/FunctionExecutor/FunctionExecutorGenerator.ExecutableFunction.cs Show resolved Hide resolved
sdk/Sdk.Generators/FunctionExecutor/FunctionExecutorGenerator.Emitter.cs Outdated
if (string.Equals(context.FunctionDefinition.EntryPoint, ""{function.EntryPoint}"", StringComparison.OrdinalIgnoreCase))
{{");

int constructorParamCounter = 0;
Copy link
Member

Choose a reason for hiding this comment

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

The activation process should be consistent with what we have today. That will also handle constructor selection and type injection (i.e., continue to go through IFunctionActivator).

Expansion to optimize activation can use the existing extensibility, but should be done separately

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Updated to use IFunctionActovator.

@kshyju kshyju requested a review from fabiocav March 11, 2023 02:25
@kshyju
Finally a green test for happy use case.
8cd05c3
@kshyju
WIP
d034e84
@kshyju
Handing async and void cases. More tests.
16506fc
@kshyju
🙈
413c29b
@kshyju
🙊
ee27f7e
@kshyju
🙉
eeb48ca
@kshyju
Clean up and addressing some PR feedback
4f81234
@kshyju
Added the extension method to register the executor to service collec…
355904f 
…tion.
@kshyju
Cleanup
c020f38
@kshyju
Rebased on main. Fixed package versions (to bump on only preview)
ca4a39d
@kshyju
Minor cleanup
1296197
@kshyju
Updating tests to reflect the renaming of the public types (IInputBin…
8e9f5f0 
…dingFeature)
@kshyju
Cleanup after rebasing on main.
90195ee
@kshyju
Removed an unused line (from rebase merge)
b8f3882
@kshyju
nit fixes based on PR feedback.
fa74060
@kshyju
Addressing PR feedback. raw literal string indendation is still drivi…
9ffdcaf 
…ng me crazy!
@kshyju
Changing the build prop to "FunctionsEnableExecutorSourceGen"
d5615bb
@kshyju
Bit of a cleanup. Removed an unused type.
36421be
@kshyju kshyju force-pushed the shkr/1284_srcgen-default-executor branch from a5e1c1f to 36421be Compare March 12, 2023 19:04
@kshyju kshyju merged commit d3822f2 into main Mar 22, 2023
@kshyju kshyju deleted the shkr/1284_srcgen-default-executor branch March 22, 2023 19:04
@satvu satvu mentioned this pull request Apr 5, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madelinegordon madelinegordon madelinegordon left review comments

@liliankasem liliankasem liliankasem approved these changes

@satvu satvu satvu approved these changes

@brettsam brettsam Awaiting requested review from brettsam

@jviau jviau Awaiting requested review from jviau

@fabiocav fabiocav Awaiting requested review from fabiocav

Assignees

@brettsam brettsam

@fabiocav fabiocav

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Source Gen] Generate function executor code
6 participants
@kshyju @satvu @liliankasem @fabiocav @madelinegordon @brettsam