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][aot] Enabling AOT wrappers for virtual delegates #85643

Merged
merged 21 commits into from
May 22, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/mono/mono/metadata/loader-internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,12 @@ struct _MonoDllMap {
#endif

typedef struct {
GHashTable *delegate_invoke_cache;
GHashTable *delegate_invoke_virtual_cache;
/*
* indexed by MonoMethodSignature
* Protected by the marshal lock
*/
GHashTable *delegate_invoke_cache;
GHashTable *delegate_begin_invoke_cache;
GHashTable *delegate_end_invoke_cache;
GHashTable *runtime_invoke_signature_cache;
Expand Down
12 changes: 9 additions & 3 deletions src/mono/mono/metadata/marshal-lightweight.c
Original file line number Diff line number Diff line change
Expand Up @@ -1990,7 +1990,7 @@ emit_delegate_end_invoke_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *sig)
}

static void
emit_delegate_invoke_internal_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *sig, MonoMethodSignature *invoke_sig, MonoMethodSignature *target_method_sig, gboolean static_method_with_first_arg_bound, gboolean callvirt, gboolean closed_over_null, MonoMethod *method, MonoMethod *target_method, MonoClass *target_class, MonoGenericContext *ctx, MonoGenericContainer *container)
emit_delegate_invoke_internal_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *sig, MonoMethodSignature *invoke_sig, MonoMethodSignature *target_method_sig, gboolean static_method_with_first_arg_bound, gboolean callvirt, gboolean closed_over_null, MonoMethod *method, MonoMethod *target_method, MonoGenericContext *ctx, MonoGenericContainer *container)
{
int local_i, local_len, local_delegates, local_d, local_target, local_res = 0;
int pos0, pos1, pos2;
Expand Down Expand Up @@ -2088,9 +2088,15 @@ emit_delegate_invoke_internal_ilgen (MonoMethodBuilder *mb, MonoMethodSignature

if (callvirt) {
if (!closed_over_null) {
for (i = 1; i <= sig->param_count; ++i)
for (i = 1; i <= sig->param_count; ++i) {
mono_mb_emit_ldarg (mb, i);
mono_mb_emit_ldarg (mb, 1);
if (i == 1 && MONO_TYPE_ISSTRUCT (sig->params [0]))
mono_mb_emit_op (mb, CEE_BOX, mono_class_from_mono_type_internal (sig->params [0]));
}
if (MONO_TYPE_ISSTRUCT (sig->params [0]))
mono_mb_emit_ldarg_addr (mb, 1);
else
mono_mb_emit_ldarg (mb, 1);
mono_mb_emit_ldarg (mb, 0);
mono_mb_emit_icall (mb, mono_get_addr_compiled_method);
mono_mb_emit_op (mb, CEE_CALLI, target_method_sig);
Expand Down
30 changes: 11 additions & 19 deletions src/mono/mono/metadata/marshal.c
Original file line number Diff line number Diff line change
Expand Up @@ -2092,7 +2092,6 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
GHashTable *cache;
gpointer cache_key = NULL;
char *name;
MonoClass *target_class = NULL;
gboolean closed_over_null = FALSE;
MonoGenericContext *ctx = NULL;
MonoGenericContainer *container = NULL;
Expand All @@ -2113,25 +2112,14 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
*/
if (callvirt) {
subtype = WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL;
if (target_method->is_inflated) {
ERROR_DECL (error);
MonoType *target_type;

g_assert (method->signature->hasthis);
target_type = mono_class_inflate_generic_type_checked (method->signature->params [0],
mono_method_get_context (method), error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
target_class = mono_class_from_mono_type_internal (target_type);
} else {
target_class = target_method->klass;
}

closed_over_null = sig->param_count == mono_method_signature_internal (target_method)->param_count;

/*
* We don't want to use target_method's signature because it can be freed early
*/
target_method_sig = mono_method_signature_internal (target_method);
target_method_sig = mono_metadata_signature_dup_delegate_invoke_to_target (invoke_sig);
// target_method_sig = mono_method_signature_internal (target_method);

closed_over_null = sig->param_count == target_method_sig->param_count;
}

if (static_method_with_first_arg_bound) {
Expand All @@ -2150,7 +2138,7 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
/*
* For generic delegates, create a generic wrapper, and return an instance to help AOT.
*/
if (method->is_inflated && subtype == WRAPPER_SUBTYPE_NONE) {
if (method->is_inflated && (subtype == WRAPPER_SUBTYPE_NONE || subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL)) {
ctx = &((MonoMethodInflated*)method)->context;
method = ((MonoMethodInflated*)method)->declaring;

Expand All @@ -2166,7 +2154,10 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
* Check cache
*/
if (ctx) {
cache = get_cache (&((MonoMethodInflated*)orig_method)->owner->wrapper_caches.delegate_invoke_cache, mono_aligned_addr_hash, NULL);
if (callvirt)
cache = get_cache (&((MonoMethodInflated*)orig_method)->owner->wrapper_caches.delegate_invoke_virtual_cache, mono_aligned_addr_hash, NULL);
else
cache = get_cache (&((MonoMethodInflated*)orig_method)->owner->wrapper_caches.delegate_invoke_cache, mono_aligned_addr_hash, NULL);
res = check_generic_delegate_wrapper_cache (cache, orig_method, method, ctx);
if (res)
return res;
Expand Down Expand Up @@ -2241,7 +2232,7 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
/* FIXME: Other subtypes */
mb->mem_manager = m_method_get_mem_manager (method);

get_marshal_cb ()->emit_delegate_invoke_internal (mb, sig, invoke_sig, target_method_sig, static_method_with_first_arg_bound, callvirt, closed_over_null, method, target_method, target_class, ctx, container);
get_marshal_cb ()->emit_delegate_invoke_internal (mb, sig, invoke_sig, target_method_sig, static_method_with_first_arg_bound, callvirt, closed_over_null, method, target_method, ctx, container);

get_marshal_cb ()->mb_skip_visibility (mb);

Expand Down Expand Up @@ -6384,6 +6375,7 @@ void
mono_wrapper_caches_free (MonoWrapperCaches *cache)
{
free_hash (cache->delegate_invoke_cache);
free_hash (cache->delegate_invoke_virtual_cache);
free_hash (cache->delegate_begin_invoke_cache);
free_hash (cache->delegate_end_invoke_cache);
free_hash (cache->delegate_bound_static_invoke_cache);
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/metadata/marshal.h
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ typedef struct {
void (*emit_runtime_invoke_dynamic) (MonoMethodBuilder *mb);
void (*emit_delegate_begin_invoke) (MonoMethodBuilder *mb, MonoMethodSignature *sig);
void (*emit_delegate_end_invoke) (MonoMethodBuilder *mb, MonoMethodSignature *sig);
void (*emit_delegate_invoke_internal) (MonoMethodBuilder *mb, MonoMethodSignature *sig, MonoMethodSignature *invoke_sig, MonoMethodSignature *target_method_sig, gboolean static_method_with_first_arg_bound, gboolean callvirt, gboolean closed_over_null, MonoMethod *method, MonoMethod *target_method, MonoClass *target_class, MonoGenericContext *ctx, MonoGenericContainer *container);
void (*emit_delegate_invoke_internal) (MonoMethodBuilder *mb, MonoMethodSignature *sig, MonoMethodSignature *invoke_sig, MonoMethodSignature *target_method_sig, gboolean static_method_with_first_arg_bound, gboolean callvirt, gboolean closed_over_null, MonoMethod *method, MonoMethod *target_method, MonoGenericContext *ctx, MonoGenericContainer *container);
void (*emit_synchronized_wrapper) (MonoMethodBuilder *mb, MonoMethod *method, MonoGenericContext *ctx, MonoGenericContainer *container, MonoMethod *enter_method, MonoMethod *exit_method, MonoMethod *gettypefromhandle_method);
void (*emit_unbox_wrapper) (MonoMethodBuilder *mb, MonoMethod *method);
void (*emit_array_accessor_wrapper) (MonoMethodBuilder *mb, MonoMethod *method, MonoMethodSignature *sig, MonoGenericContext *ctx);
Expand Down
1 change: 1 addition & 0 deletions src/mono/mono/metadata/metadata-internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,7 @@ MonoMethodSignature *mono_metadata_signature_dup_full (MonoImage *image,MonoMet
MonoMethodSignature *mono_metadata_signature_dup_mempool (MonoMemPool *mp, MonoMethodSignature *sig);
MonoMethodSignature *mono_metadata_signature_dup_mem_manager (MonoMemoryManager *mem_manager, MonoMethodSignature *sig);
MonoMethodSignature *mono_metadata_signature_dup_add_this (MonoImage *image, MonoMethodSignature *sig, MonoClass *klass);
MonoMethodSignature *mono_metadata_signature_dup_delegate_invoke_to_target (MonoMethodSignature *sig);

MonoGenericInst *
mono_get_shared_generic_inst (MonoGenericContainer *container);
Expand Down
27 changes: 27 additions & 0 deletions src/mono/mono/metadata/metadata.c
Original file line number Diff line number Diff line change
Expand Up @@ -2512,6 +2512,33 @@ mono_metadata_signature_dup (MonoMethodSignature *sig)
return mono_metadata_signature_dup_full (NULL, sig);
}

/**
* mono_metadata_signature_dup_delegate_invoke_to_target:
* \param sig method signature
*
* Duplicate an existing \c MonoMethodSignature but removes first param from it so it can
* be used as signature for a delegate target method.
* This is a Mono runtime internal function.
*
* \returns the new \c MonoMethodSignature structure.
*/
MonoMethodSignature*
mono_metadata_signature_dup_delegate_invoke_to_target (MonoMethodSignature *sig)
{
MonoMethodSignature *res = mono_metadata_signature_dup_full (NULL, sig);

for (int i = 0 ; i < sig->param_count - 1; i ++) {
res->params [i] = sig->params [i + 1];
}

res->param_count --;
kotlarmilos marked this conversation as resolved.
Show resolved Hide resolved

// if (res->params [res->param_count - 1])
// mono_metadata_free_type (res->params [res->param_count --]);

return res;
}

/*
* mono_metadata_signature_size:
*
Expand Down
71 changes: 67 additions & 4 deletions src/mono/mono/mini/aot-compiler.c
Original file line number Diff line number Diff line change
Expand Up @@ -3922,12 +3922,16 @@ encode_method_ref (MonoAotCompile *acfg, MonoMethod *method, guint8 *buf, guint8
encode_klass_ref (acfg, method->klass, p, &p);
} else {
MonoMethodSignature *sig = mono_method_signature_internal (method);
WrapperInfo *wrapper_info = mono_marshal_get_wrapper_info (method);
// WrapperInfo *wrapper_info = mono_marshal_get_wrapper_info (method);

encode_value (0, p, &p);
if (method->wrapper_type == MONO_WRAPPER_DELEGATE_INVOKE)
encode_value (wrapper_info ? wrapper_info->subtype : 0, p, &p);
encode_signature (acfg, sig, p, &p);
encode_value (info ? info->subtype : 0, p, &p);

if (info && info->subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL)
encode_klass_ref (acfg, info->d.delegate_invoke.method->klass, p, &p);
else
vargaz marked this conversation as resolved.
Show resolved Hide resolved
encode_signature (acfg, sig, p, &p);
}
break;
}
Expand Down Expand Up @@ -4546,6 +4550,39 @@ can_marshal_struct (MonoClass *klass)
return can_marshal;
}

/* Create a ref shared instantiation */
static void
create_ref_shared_inst (MonoAotCompile *acfg, MonoMethod *method, MonoGenericContext *ctx)
{
MonoGenericContext shared_context;
MonoType **args;
MonoGenericInst *inst;
MonoGenericContainer *container;

memset (ctx, 0, sizeof (MonoGenericContext));

if (mono_class_is_gtd (method->klass)) {
shared_context = mono_class_get_generic_container (method->klass)->context;
inst = shared_context.class_inst;

args = g_new0 (MonoType*, inst->type_argc);
Copy link
Member

Choose a reason for hiding this comment

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

Is the memory being deallocated correctly?

Copy link
Contributor Author

@LeVladIonescu LeVladIonescu May 16, 2023

Choose a reason for hiding this comment

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

No, it is not. I would suggest deallocating it after

memcpy (ginst->type_argv, type_argv, type_argc * sizeof (MonoType *));

^ @vargaz

Copy link
Contributor

Choose a reason for hiding this comment

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

I can be freed, but the aot compiler is already leaking memory in a number of places, so its not that important.

for (guint i = 0; i < inst->type_argc; ++i)
args [i] = mono_get_object_type ();
ctx->class_inst = mono_metadata_get_generic_inst (inst->type_argc, args);
}
if (method->is_generic) {
container = mono_method_get_generic_container (method);
g_assert (!container->is_anonymous && container->is_method);
shared_context = container->context;
inst = shared_context.method_inst;

args = g_new0 (MonoType*, inst->type_argc);
for (int i = 0; i < container->type_argc; ++i)
args [i] = mono_get_object_type ();
ctx->method_inst = mono_metadata_get_generic_inst (inst->type_argc, args);
}
}

static void
create_gsharedvt_inst (MonoAotCompile *acfg, MonoMethod *method, MonoGenericContext *ctx)
{
Expand Down Expand Up @@ -4962,13 +4999,39 @@ add_full_aot_wrappers (MonoAotCompile *acfg)
if (!m_class_is_delegate (klass) || klass == mono_defaults.delegate_class || klass == mono_defaults.multicastdelegate_class)
continue;

method = mono_get_delegate_invoke_internal (klass);
if (mono_class_is_gtd (klass)) {
MonoGenericContext ctx;
MonoMethod *inst, *gshared;

create_ref_shared_inst (acfg, method, &ctx);

inst = mono_class_inflate_generic_method_checked (method, &ctx, error);
g_assert (is_ok (error)); /* FIXME don't swallow the error */

sig = mono_method_signature_internal (method);
if (sig->param_count && !m_class_is_byreflike (mono_class_from_mono_type_internal (sig->params [0])) && !m_type_is_byref (sig->params [0]))
m = mono_marshal_get_delegate_invoke_internal (inst, TRUE, FALSE, NULL);

gshared = mini_get_shared_method_full (m, SHARE_MODE_NONE, error);
mono_error_assert_ok (error);

add_extra_method (acfg, gshared);
}
}

if (!mono_class_is_gtd (klass)) {
method = mono_get_delegate_invoke_internal (klass);

m = mono_marshal_get_delegate_invoke (method, NULL);

add_method (acfg, m);

sig = mono_method_signature_internal (method);
if (sig->param_count && !m_class_is_byreflike (mono_class_from_mono_type_internal (sig->params [0])) && !m_type_is_byref (sig->params [0]))
m = mono_marshal_get_delegate_invoke_internal (method, TRUE, FALSE, NULL);
add_method (acfg, m);
}

method = try_get_method_nofail (klass, "BeginInvoke", -1, 0);
if (method)
add_method (acfg, mono_marshal_get_delegate_begin_invoke (method));
Expand Down
24 changes: 17 additions & 7 deletions src/mono/mono/mini/aot-runtime.c
Original file line number Diff line number Diff line change
Expand Up @@ -1269,15 +1269,25 @@ decode_method_ref_with_target (MonoAotModule *module, MethodRef *ref, MonoMethod
ref->method = wrapper;
}
} else {
/*
* These wrappers are associated with a signature, not with a method.
* Since we can't decode them into methods, they need a target method.
*/
if (!target)
vargaz marked this conversation as resolved.
Show resolved Hide resolved
return FALSE;
MonoClass *klass;
MonoMethod *invoke;


if (wrapper_type == MONO_WRAPPER_DELEGATE_INVOKE) {
subtype = (WrapperSubtype)decode_value (p, &p);
if (subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL) {
klass = decode_klass_ref (module, p, &p, error);
invoke = mono_get_delegate_invoke_internal (klass);
ref->method = mono_marshal_get_delegate_invoke_internal(invoke, TRUE, FALSE, NULL);
break;
}

/*
* These wrappers are associated with a signature, not with a method.
* Since we can't decode them into methods, they need a target method.
*/
if (!target)
return FALSE;
info = mono_marshal_get_wrapper_info (target);
if (info) {
if (info->subtype != subtype)
Expand All @@ -1287,7 +1297,7 @@ decode_method_ref_with_target (MonoAotModule *module, MethodRef *ref, MonoMethod
return FALSE;
}
}
if (sig_matches_target (module, target, p, &p))
if (target && sig_matches_target (module, target, p, &p))
ref->method = target;
else
return FALSE;
Expand Down