Skip to content

[mono] Precompute the CallInfo structure used by the mono_arch_..nati… - #88369

Merged
vargaz merged 3 commits into
dotnet:mainfrom
vargaz:native-call-context
Jul 10, 2023
Merged

[mono] Precompute the CallInfo structure used by the mono_arch_..nati…#88369
vargaz merged 3 commits into
dotnet:mainfrom
vargaz:native-call-context

Conversation

@vargaz

@vargazvargaz commented Jul 4, 2023

Copy link
Copy Markdown
Contributor

…ve_call_context_.. functions.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 4, 2023
@ghostghost assigned vargazJul 4, 2023
@vargaz

vargaz commented Jul 4, 2023

Copy link
Copy Markdown
ContributorAuthor

This speeds up ves_pinvoke_method () in the interpreter, including the icalls which use QCallTypeHandle.

@vargaz
vargazforce-pushed the native-call-context branch from 2c05979 to 74f0f53CompareJuly 4, 2023 05:18
BrzVlad
BrzVlad reviewed Jul 4, 2023
View reviewed changes
Comment threadsrc/mono/mono/mini/interp/interp.c
if (!call_info) {
call_info = mono_arch_get_interp_native_call_info (get_default_mem_manager (), sig);
mono_memory_barrier ();
*cache = call_info;

@BrzVladBrzVladJul 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use a CAS here to avoid leaks from races

@vargazvargazJul 4, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The memory is allocated from the mem manager, so its going to be leaked anyway.

@BrzVladBrzVladJul 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Also a mono_memory_write_barrier here is enough.

BrzVlad
BrzVlad reviewed Jul 4, 2023
View reviewed changes
Comment threadsrc/mono/mono/mini/mini-amd64.c Outdated
BrzVlad
BrzVlad reviewed Jul 4, 2023
View reviewed changes
Comment threadsrc/mono/mono/mini/mini-arm.c Outdated
BrzVlad
BrzVlad reviewed Jul 4, 2023
View reviewed changes
Comment threadsrc/mono/mono/mini/mini-arm64.c Outdated
@vargaz

vargaz commented Jul 4, 2023

Copy link
Copy Markdown
ContributorAuthor

This speeds up this testcase by about 50% when running with the interpreter:

 for (int i = 0; i < 10000000; ++i) {
var t = typeof (int).DeclaringType;
}

@danmoseleydanmoseley added area-VM-meta-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 4, 2023
@vargaz
vargaz merged commit 09f4951 into dotnet:mainJul 10, 2023
@vargaz
vargaz deleted the native-call-context branch July 10, 2023 23:41
@BrzVladBrzVlad mentioned this pull request Jul 13, 2023
Closed
@kotlarmiloskotlarmilos mentioned this pull request Jul 19, 2023
Closed
@kotlarmiloskotlarmilos mentioned this pull request Aug 11, 2023
Closed
@ghostghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@BrzVladBrzVladBrzVlad left review comments
@lewinglewinglewing approved these changes
@kotlarmiloskotlarmilosAwaiting requested review from kotlarmiloskotlarmilos is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@SamMonoRTSamMonoRTAwaiting requested review from SamMonoRT

Assignees

@vargazvargaz

Labels

area-VM-meta-mono

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vargaz@lewing@BrzVlad@danmoseley