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] reduce eval in production code #71932

Merged
merged 3 commits into from
Jul 11, 2022

Conversation

pavelsavara
Copy link
Member

Fixes #61287

@pavelsavara pavelsavara added this to the 7.0.0 milestone Jul 11, 2022
@pavelsavara pavelsavara requested a review from maraf July 11, 2022 10:43
@pavelsavara pavelsavara requested a review from lewing as a code owner July 11, 2022 10:43
@pavelsavara pavelsavara self-assigned this Jul 11, 2022
@ghost
Copy link

ghost commented Jul 11, 2022

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

Issue Details

Fixes #61287

Author: pavelsavara
Assignees: pavelsavara
Labels:

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

Milestone: 7.0.0

@pavelsavara
Copy link
Member Author

cc @kg

- update benchmark to use JSExport/JSImport instead of reflection
@pavelsavara
Copy link
Member Author

Sorry guys, I realized that there is still usage in the benchmark.
That also forced me to fix JSExport from nested class.

@pavelsavara pavelsavara merged commit 4a2ebf2 into dotnet:main Jul 11, 2022
@pavelsavara pavelsavara deleted the wasm_reduce_eval branch July 14, 2022 20:48
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2022
@kg
Copy link
Member

kg commented Jan 25, 2023

https://radekdoulik.github.io/WasmPerformanceMeasurements/?startDate=2022-07-11T19%3A31%3A11.000Z&endDate=2022-07-11T21%3A06%3A41.000Z&tasks=%2CAppStart&flavors=2

The time increase in this doesn't make any sense given the change in question, seems like variance?

@lewing
Copy link
Member

lewing commented Jan 25, 2023

cae954a...4a2ebf2 is the only commit in the diff and it changes the test mechanism. it was known and mostly fixed with the lazy init I think

@pavelsavara
Copy link
Member Author

pavelsavara commented Jan 25, 2023

Yes, I had to rewrite the benchmark to stop using eval code path and so I introduced dependency on JSExport into benchmark. The implementation at the time was triggering static constructors of the containing class during runtime init. That's now fixed in #77293

@radekdoulik
Copy link
Member

https://radekdoulik.github.io/WasmPerformanceMeasurements/?startDate=2022-07-11T19%3A31%3A11.000Z&endDate=2022-07-11T21%3A06%3A41.000Z&tasks=%2CAppStart&flavors=2

yup, it is #75598 and Pavel fixed it in #77293. the static constructors were doing a lot of work, which was not needed during startup

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.

[wasm] avoid eval, design better API for InvokeJS
5 participants