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

[wasm] fix aotProfiler init #79651

Merged
merged 2 commits into from
Dec 15, 2022
Merged

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Dec 14, 2022

This is also wrong on Net7 but this code path via cwraps was not used and it worked via ccall.
We don't need to backport it.

@pavelsavara pavelsavara added this to the 8.0.0 milestone Dec 14, 2022
@pavelsavara pavelsavara requested a review from maraf December 14, 2022 15:11
@pavelsavara pavelsavara requested a review from lewing as a code owner December 14, 2022 15:11
@pavelsavara pavelsavara self-assigned this Dec 14, 2022
@ghost
Copy link

ghost commented Dec 14, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

This is also wrong on Net7 but I think we don't need to backport it.

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 8.0.0

@radical
Copy link
Member

radical commented Dec 14, 2022

How does this manifest for a user?

@radical
Copy link
Member

radical commented Dec 14, 2022

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@pavelsavara
Copy link
Member Author

this sample should run and trigger download file in your browser src\mono\sample\wasm\browser-profile\
without the fix it will just fail

@radical
Copy link
Member

radical commented Dec 14, 2022

this sample should run and trigger download file in your browser src\mono\sample\wasm\browser-profile\
without the fix it will just fail

Oh, and it's not used by anything else right now? Or I guess not by 7.0 users, thus not backporting?

@radical
Copy link
Member

radical commented Dec 14, 2022

/azp run runtime,runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@pavelsavara
Copy link
Member Author

Oh, and it's not used by anything else right now? Or I guess not by 7.0 users, thus not backporting?

Thanks for asking @radical , I did bit more research.

So, the method had different name in Net7 it was mono_wasm_load_profiler_aot and it had number in the cwraps signature.
But the cwraps signature was not used and there was ccall in the profiler.ts with string signature.
And it worked, so we don't need to backport.

I changed it in #77779 and broken it without noticing it (which is Net8).
And this PR fixes it.

We should eventually add some WBT+playwrite to cover more samples or more scenarios.
But I plan to work on AOT profiler bit more now, so maybe it would be too early to cover it with test now.

@pavelsavara
Copy link
Member Author

The error is unrelated

@pavelsavara pavelsavara merged commit cddf579 into dotnet:main Dec 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2023
@pavelsavara pavelsavara deleted the wasm_fix_aot_profiler branch September 2, 2024 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants