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

[mono] Allow a single call when inlining methods #83548

Merged
merged 4 commits into from
Mar 17, 2023

Conversation

kg
Copy link
Contributor

@kg kg commented Mar 16, 2023

This combined with #83490 would allow inlining List.get_Item, since it contains a single call to a ThrowHelper.
DoesNotReturn method calls also no longer prohibit inlining, which will help for methods that call ThrowHelper in a never-taken* branch and then make a single call that does actual work.

@ghost ghost assigned kg Mar 16, 2023
@kg kg changed the title Allow a single call when inlining methods [mono] Allow a single call when inlining methods Mar 16, 2023
@ghost
Copy link

ghost commented Mar 16, 2023

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

This combined with #83490 would allow inlining List.get_Item, since it contains a single call to a ThrowHelper.
In the future I want to also exempt DoesNotReturn methods from this rule entirely.

Author: kg
Assignees: -
Labels:

area-Codegen-Interpreter-mono

Milestone: -

@kg
Copy link
Contributor Author

kg commented Mar 16, 2023

Doesn't work right because I need to save/restore the TransformData flag when inlining, I think. Looking into it.

@kg kg added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 16, 2023
…hods

Don't allow doesnotreturn method calls to disable inlining, since we know they are unlikely to be called
@kg kg removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 16, 2023
@kg
Copy link
Contributor Author

kg commented Mar 16, 2023

Fixed it, should be ready for review now. In my testing it takes the V8.Crypto benchmark from dotnet/performance down to ~180ms from a baseline of 260-280ms.

EDIT: Previous measurement (~80ms) was combined with #83490, this by itself is ~180ms

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.

2 participants