-
Notifications
You must be signed in to change notification settings - Fork 287
Setup new dotnet new android measurements using Mono or CoreCLR runtimes #4770
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
Setup new dotnet new android measurements using Mono or CoreCLR runtimes #4770
Conversation
- Use latest .NET sdk and Android SDK from dotnet10 feed - Setup `dotnet new android` for CoreCLR
bfe1b65 to
de4ab6e
Compare
caaavik-msft
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.
Looks good to me. One question I have, are any of these changes (e.g. the _MSBuildArgs) being made because they are coupled to the runtime repository itself or would you want them set even if we decided to run against an older version of the runtime?
As part of the recent changes we made to move the yaml from the runtime to the performance repository, I'd like to know if any of the things we moved over were better living in the runtime repository. If so, we can look at restructuring where these live to better couple related changes.
They are not coupled with the dotnet/runtime repository but rather with dotnet/android that use them to select/configure appropriate runtime (Mono vs CoreCLR).
We won't be able to run CoreCLR Android perf jobs against older runtime because the runtime doesn't support it. For Mono, it would work as is.
I think it is ideal that all this now lives in dotnet/performance as it doesn't necessary depend on any custom/local builds of runtime. Also it is much simpler to debug now as these specific jobs all run on a single pipeline. |
| <ApkName>com.companyname.mauiandroiddefault-Signed</ApkName> | ||
| <PackageName>com.companyname.mauiandroiddefault</PackageName> | ||
| </MAUIAndroidScenario> | ||
| <MAUIAndroidScenario Include="Maui Blazor Android Default (TTFD) $(RunConfigsString)" Condition="'$(RuntimeFlavor)' == 'mono'"> |
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 only care about TTFD measurement here? Or why don't we measure both TTID and TTFD?
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.
I don't know. I just saw that it is passing the --use-fully-drawn-time so I added it to the scenario name to make it clear. I can probably add TTID as well if this is something we want.
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.
@LoopedBard3 do you remember why for blazor we are using --use-fully-drawn-time and not the default startup measurement config?
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.
I don't recall anything specifically. Maybe TTID doesn't include some of the blazor startup. Regardless, I don't see any reason not to include both.
|
There might be some issues with the approach of Will experiment a bit with this but I think we might need to the the |
7bd1539
7bd1539 to
1a0f303
Compare
This reverts commit a35ebc3.
LoopedBard3
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.
This looks really great! I am excited to have what seems to be a more robust solution for building the apps. I left one comment but am also wondering if this supports 9.0 and 8.0 mobile app runs. It seems that it currently sets up the runs to only run main.
Yes, currently works just on main. I think it could be extended to other branches. We would just search a different dotnet feed I think and make it more parametrized. However, I don't see much value now in having measurements on other branches but we could extend it in the future if needed. |
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.
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- eng/performance/maui_scenarios_android.proj: Language not supported
Comments suppressed due to low confidence (1)
eng/pipelines/sdk-perf-jobs.yml:405
- [nitpick] The pipeline uses 'codeGenType' (PascalCase) while the corresponding code in run_performance_job.py references 'codegen_type' (lowercase). Consider using consistent naming conventions.
codeGenType: ProfiledAOT
| The latest feed is the one that has the highest version number. | ||
| Supports only single number versioning (dotnet9, dotnet10, etc.) | ||
| ''' | ||
| tree = ET.parse(path) |
Copilot
AI
Apr 10, 2025
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 function 'extract_latest_dotnet_feed_from_nuget_config' uses 'ET' without an import. Please add 'import xml.etree.ElementTree as ET' at the top of the file.
| key = add_element.get("key") | ||
| value = add_element.get("value") | ||
| if key and value: | ||
| match = re.match(r"dotnet(\d+)$", key) |
Copilot
AI
Apr 10, 2025
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 regex usage 're.match' requires the 're' module but no import is present. Please add 'import re' at the beginning of the file.
ivanpovazan
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.
This looks neat, thank you!
The only thoughts left (not for this PR) are:
- It seems like the rollback file could be provided by some other service rather than us manually creating it as part of performance runs
- Think about a way of doing performance runs on specific releases, versions i.e., answering a question: what is the performance of our preview.X release - and with that freeze/tag such measurements for reference
Other than that, really nice work!
We can do that locally by using manually installed dotnet and providing |
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.
LGTM, just the one minor comment on the yaml condition
Co-authored-by: Cameron Aavik <99771732+caaavik-msft@users.noreply.github.com>
c5ee46f
… use a new flow (dotnet#4770). Also left a comment for the two main functions that used the references incase we need them in the future.
Switching MAUI-Android (and .NET Android) scenarios to use latest .NET SDK + dotnet10 feed, the steps are:
.dotnet-install.sh --channel 10.0 --quality dailydotnet package search.Adding new jobs:
dotnet new androidrunning CoreCLR.dotnet new maui(Android) running CoreCLR.dotnet new maui-blazor(Android) running CoreCLR.Internal pipeline run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2684265
TODO:
Nuget.config(should be added by maestro)Out of scope
#4754