-
Notifications
You must be signed in to change notification settings - Fork 488
Snapstart Minimal API Performance Improvements #2010
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
base: dev
Are you sure you want to change the base?
Conversation
Libraries/src/Amazon.Lambda.RuntimeSupport/Client/RuntimeApiHeaders.cs
Outdated
Show resolved
Hide resolved
...mbda.AspNetCoreServer.Hosting/Internal/LambdaSnapstartExecuteRequestsBeforeSnapshotHelper.cs
Outdated
Show resolved
Hide resolved
Libraries/src/Amazon.Lambda.AspNetCoreServer.Hosting/Internal/LambdaRuntimeSupportServer.cs
Outdated
Show resolved
Hide resolved
…et/lambda pipelines automatically during BeforeSnapshot callback.
520905f
to
784e195
Compare
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.
@normj I don't know if there's an implication of Strong Name signing the RuntimeSupport library. Not sure if there are any ramifications for Lambda and our customers.
Ran the AWS CI PR check manually https://github.com/aws/aws-lambda-dotnet/actions/runs/13835574482/job/38709627134 |
...ies/src/Amazon.Lambda.AspNetCoreServer.Hosting/Amazon.Lambda.AspNetCoreServer.Hosting.csproj
Outdated
Show resolved
Hide resolved
Libraries/src/Amazon.Lambda.AspNetCoreServer.Hosting/Internal/LambdaRuntimeSupportServer.cs
Show resolved
Hide resolved
...mbda.AspNetCoreServer.Hosting/Internal/LambdaSnapstartExecuteRequestsBeforeSnapshotHelper.cs
Show resolved
Hide resolved
...mbda.AspNetCoreServer.Hosting/Internal/LambdaSnapstartExecuteRequestsBeforeSnapshotHelper.cs
Outdated
Show resolved
Hide resolved
...mbda.AspNetCoreServer.Hosting/Internal/LambdaSnapstartExecuteRequestsBeforeSnapshotHelper.cs
Show resolved
Hide resolved
...mbda.AspNetCoreServer.Hosting/Internal/LambdaSnapstartExecuteRequestsBeforeSnapshotHelper.cs
Outdated
Show resolved
Hide resolved
Libraries/src/Amazon.Lambda.RuntimeSupport/Amazon.Lambda.RuntimeSupport.csproj
Outdated
Show resolved
Hide resolved
...mbda.AspNetCoreServer.Hosting/Internal/LambdaSnapstartExecuteRequestsBeforeSnapshotHelper.cs
Show resolved
Hide resolved
3df5501
to
fa3658e
Compare
fa3658e
to
fc98552
Compare
…ng json serialization, adjust #if blocks and fix simple warnings
…sibileTo Amazon.Lambda.AspNetCorServer.Hosting
…ing calls to HttpClient
e3a87f5
to
c7d6057
Compare
c7d6057
to
b476fcd
Compare
@@ -16,6 +16,13 @@ | |||
<LangVersion>9.0</LangVersion> | |||
</PropertyGroup> | |||
|
|||
<PropertyGroup> |
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 should be removable now that you got rid of the internal visible to and the strong naming is already being controlled via the <repo-root>/buildtools/common.props
.
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.
My reading is there are 2 different solutions being implemented here. One for AspNetCoreServer.Hosting and one for just using AspNetCoreServer. I would rather we not have different solutions and it seems like the subclasses in AspNetCoreServer.Hosting could override the virtual method for getting the list or requests to run and the service collection extension method could provide a way to get the request into that collection returned in the override.
Plus if you do it that way you can remove any changes you did to RuntimeSupport.
@@ -31,6 +31,7 @@ | |||
|
|||
<ItemGroup> | |||
<FrameworkReference Include="Microsoft.AspNetCore.App" /> | |||
<ProjectReference Include="..\Amazon.Lambda.RuntimeSupport\Amazon.Lambda.RuntimeSupport.csproj" /> |
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.
From what I can tell this is only need for SnapStartEmptyLambdaContext
to get the name environment variables. This is a big dependency being added that users using only this library and not hosting don't need. This could cause a performance regression for non snapstart use cases. We should not add this here and copy the constants used in runtime support for looking up the environment variables in SnapStartEmptyLambdaContext
.
return await r.Content.ReadAsStringAsync(); | ||
} | ||
|
||
[JsonSourceGenerationOptions(WriteIndented = 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 indented? That seems unnecessary bloat when in production.
[JsonSerializable(typeof(ApplicationLoadBalancerRequest))] | ||
[JsonSerializable(typeof(APIGatewayHttpApiV2ProxyRequest))] | ||
[JsonSerializable(typeof(APIGatewayProxyRequest))] | ||
[JsonSerializable(typeof(APIGatewayProxyRequest.ClientCertValidity))] |
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 do we need these inner types. Shouldn't having APIGatewayProxyRequest
cover all of these cases?
[JsonSerializable(typeof(APIGatewayProxyRequest.ProxyRequestContext))] | ||
[JsonSerializable(typeof(APIGatewayProxyRequest.RequestIdentity))] | ||
|
||
internal partial class LambdaRequestTypeClasses : JsonSerializerContext |
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 doesn't compile for me because the parent class also has to be marked as partial.
// make request absolut (relative to localhost) otherwise parsing the query will not work | ||
request.RequestUri = new Uri(_baseUri, request.RequestUri); | ||
|
||
var duckRequest = new |
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.
Using anonymous types is likely going to be a problem for Native AOT. Sadly a lot of Native AOT warnings don't surface till you actually publish with Native AOT turned on.
/// ]]> | ||
/// </code> | ||
/// </summary> | ||
protected virtual IEnumerable<HttpRequestMessage> RegisterBeforeSnapshotRequest() => |
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.
The name RegisterBeforeSnapshotRequest
throws me off since invoking it doesn't register it. Instead it returns back a list of request to run. I would think something like GetBeforeSnapshotRequests
.
|
||
var context = CreateContext(features); | ||
|
||
var lambdaContext = new SnapStartEmptyLambdaContext(); |
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.
Create a new one per iteration. The context has the time remaining which should be reset during each invocation.
public ILambdaLogger Logger => null; | ||
public string LogGroupName => _lambdaEnvironment.LogGroupName; | ||
public string LogStreamName => _lambdaEnvironment.LogStreamName; | ||
public int MemoryLimitInMB => 128; |
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.
Grab the write value from the AWS_LAMBDA_FUNCTION_MEMORY_SIZE
environment variable.
|
||
internal class SnapStartEmptyLambdaContext : ILambdaContext, ICognitoIdentity, IClientContext | ||
{ | ||
private LambdaEnvironment _lambdaEnvironment = new(); |
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.
As commented else where we should not add a dependency for RuntimeSupport on this library. So you should remove LambdaEnvironment
from here and copy the environment variable constants from RuntimeSupport.
Issue #, if available:
It is not feasible for users to warm up the asp.net and lambda pipelines via
SnapshotRestore.RegisterBeforeSnapshot
when using .NET MinimalAPI Lambda.Description of changes:
Creates a new extension method,
AddAWSLambdaBeforeSnapshotRequest
that allows users to add a function meant to initialize the asp.net and lambda pipelines duringSnapshotRestore.RegisterBeforeSnapshot
, improving the performance gains offered by SnapStart.Example:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.