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

Publish as AoT #967

Merged
merged 18 commits into from
Nov 27, 2023
Merged

Publish as AoT #967

merged 18 commits into from
Nov 27, 2023

Conversation

martincostello
Copy link
Owner

@martincostello martincostello commented Nov 25, 2023

  • Add verification tests of the output JSON.
  • Remove Application Insights as it is not AoT-compatible.
  • Use System.Text.Json for serialization instead of Newtonsoft.Json to support AoT.
  • Use invariant globalization and remove ICU package.
  • Enable publishing the Lambda for AoT.
  • Remove unused/duplicate custom JsonSerializerContext.
  • Add more end-to-end tests.

@martincostello martincostello added enhancement dependencies Pull requests that update a dependency file .NET Pull requests that update .net code labels Nov 25, 2023
Copy link

codecov bot commented Nov 25, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (e9095df) 96.65% compared to head (fc60961) 96.72%.

Files Patch % Lines
...Travel.Skill/Models/CustomStringEnumConverter`1.cs 62.50% 0 Missing and 3 partials ⚠️
...ondonTravel.Skill/Models/MixedDateTimeConverter.cs 71.42% 1 Missing and 1 partial ⚠️
src/LondonTravel.Skill/SkillResponseExtensions.cs 92.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #967      +/-   ##
==========================================
+ Coverage   96.65%   96.72%   +0.06%     
==========================================
  Files          23       45      +22     
  Lines         479      641     +162     
  Branches       54       71      +17     
==========================================
+ Hits          463      620     +157     
- Misses          5        7       +2     
- Partials       11       14       +3     
Flag Coverage Δ
linux 95.94% <96.39%> (+0.32%) ⬆️
macos 96.72% <96.39%> (+0.06%) ⬆️
windows 96.72% <96.39%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@martincostello
Copy link
Owner Author

/deploy

@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 25, 2023

🧪 Tests for deployment to dev failed ❌

@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 25, 2023

🧪 Tests for deployment to dev passed ✅

@martincostello
Copy link
Owner Author

/deploy dev

1 similar comment
@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev passed ✅

@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev passed ✅

@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev passed ✅

@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev passed ✅

@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev passed ✅

Add verification tests of the output JSON.
Remove ApplicationInsights as it is not AoT-compatible (and I never look at the data anyway).
- Use System.Text.Json for serialization instead of Newtonsoft.Json to support AoT.
- Remove unused/duplicate custom `JsonSerializerContext`.
- Add more end-to-end tests.
Enable publishing the Lambda for AoT.
Harden `SkillRequestExtensions` against null properties on the request and make it easier to debug.
Add missing brace.
Increase code coverage for ending a session.
Sort the properties.
Save some lines.
Until there's a GitHub Actions Linux runner with arm64 support, need to switch back to x64 as it is not possible to compile AoT for arm64 on an x64 Linux image.
Include the ICU libraries for linux-x64.
Make the type non-abstract to see if it fixes `NotSupportedException` when attempting to deserialize the payload.
Remove polymorphism as it is not supported when the type discriminator is not the first property in the JSON document.
See dotnet/runtime#72604.
Use invariant globalisation and remove Microsoft.ICU.ICU4C.Runtime to reduce the AoT deployment size.
@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev passed ✅

Reduce memory required to 128MB (the minimum) to see if enabling AoT allows that to work.
@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev failed ❌

Increase TfL API timeout to 7.5 seconds.
@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev passed ✅

Compare the time to the same code with 128MB.
@martincostello
Copy link
Owner Author

/deploy dev

Copy link

github-actions bot commented Nov 26, 2023

🧪 Tests for deployment to dev passed ✅

Remove the verification tests now the JSON serialization migration is complete.
@martincostello
Copy link
Owner Author

martincostello commented Nov 27, 2023

Before AoT on arm64:

Published size: 123MB; 240 files

REPORT
Duration: 3092.87 ms
Billed Duration: 3817 ms
Memory Size: 192 MB
Max Memory Used: 89 MB
Init Duration: 723.62 ms

After AoT on x86_64:

Published size: 39.1MB; 4 files
Executable size: 13.4MB

REPORT
Duration: 219.74 ms
Billed Duration: 350 ms
Memory Size: 192 MB
Max Memory Used: 31 MB
Init Duration: 129.42 ms

TL;DR:

Duration: -93%
Billed Duration: -91%
Memory Size: -
Max Memory Used: -65%
Init Duration: -82%

Published size: -68%; -236 files

@martincostello martincostello marked this pull request as ready for review November 27, 2023 09:05
@martincostello martincostello merged commit 09568ce into main Nov 27, 2023
12 of 13 checks passed
@martincostello martincostello deleted the publish-aot branch November 27, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement .NET Pull requests that update .net code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant