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] Migrate more ghashtables to simdhash #102476

Merged
merged 2 commits into from
Jun 8, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
49 changes: 27 additions & 22 deletions src/mono/mono/component/debugger-engine.c
Original file line number Diff line number Diff line change
Expand Up @@ -362,31 +362,39 @@ set_bp_in_method (MonoDomain *domain, MonoMethod *method, MonoSeqPointInfo *seq_
typedef struct {
MonoBreakpoint *bp;
GPtrArray *methods;
GPtrArray *method_domains;
GPtrArray *method_seq_points;

MonoDomain *domain;
} CollectDomainData;

static void
collect_domain_bp (gpointer key, gpointer value, gpointer user_data)
collect_domain_bp_inner (gpointer key, gpointer value, gpointer user_data)
{
GHashTableIter iter;
MonoSeqPointInfo *seq_points;
MonoDomain *domain = (MonoDomain*)key;
MonoMethod *m = (MonoMethod *)key;
MonoSeqPointInfo *seq_points = (MonoSeqPointInfo *)value;
CollectDomainData *ud = (CollectDomainData*)user_data;
MonoMethod *m;

if (!bp_matches_method (ud->bp, m))
return;

/* Save the info locally to simplify the code inside the domain lock */
g_ptr_array_add (ud->methods, m);
g_ptr_array_add (ud->method_seq_points, seq_points);
}

// HACK: Address some sort of linker problem on arm64
void
mono_jit_memory_manager_foreach_seq_point (dn_simdhash_ght_t *seq_points, dn_simdhash_ght_foreach_func func, gpointer user_data);

static void
collect_domain_bp (CollectDomainData *ud)
{
// FIXME:
MonoJitMemoryManager *jit_mm = get_default_jit_mm ();
jit_mm_lock (jit_mm);
g_hash_table_iter_init (&iter, jit_mm->seq_points);
while (g_hash_table_iter_next (&iter, (void**)&m, (void**)&seq_points)) {
if (bp_matches_method (ud->bp, m)) {
/* Save the info locally to simplify the code inside the domain lock */
g_ptr_array_add (ud->methods, m);
g_ptr_array_add (ud->method_domains, domain);
g_ptr_array_add (ud->method_seq_points, seq_points);
}
}
// FIXME: This previously used an iterator instead of foreach, so introducing
// an iterator API to simdhash might make it faster.
mono_jit_memory_manager_foreach_seq_point (jit_mm->seq_points, collect_domain_bp_inner, ud);
jit_mm_unlock (jit_mm);
}

Expand All @@ -408,7 +416,6 @@ mono_de_set_breakpoint (MonoMethod *method, long il_offset, EventRequest *req, M
MonoMethod *m;
MonoSeqPointInfo *seq_points;
GPtrArray *methods;
GPtrArray *method_domains;
GPtrArray *method_seq_points;

if (error)
Expand All @@ -429,7 +436,6 @@ mono_de_set_breakpoint (MonoMethod *method, long il_offset, EventRequest *req, M
PRINT_DEBUG_MSG (1, "[dbg] Setting %sbreakpoint at %s:0x%x.\n", (req->event_kind == EVENT_KIND_STEP) ? "single step " : "", method ? mono_method_full_name (method, TRUE) : "<all>", (int)il_offset);

methods = g_ptr_array_new ();
method_domains = g_ptr_array_new ();
method_seq_points = g_ptr_array_new ();

mono_loader_lock ();
Expand All @@ -438,20 +444,20 @@ mono_de_set_breakpoint (MonoMethod *method, long il_offset, EventRequest *req, M
memset (&user_data, 0, sizeof (user_data));
user_data.bp = bp;
user_data.methods = methods;
user_data.method_domains = method_domains;
user_data.method_seq_points = method_seq_points;
mono_de_foreach_domain (collect_domain_bp, &user_data);
user_data.domain = mono_get_root_domain ();
collect_domain_bp (&user_data);

for (guint i = 0; i < methods->len; ++i) {
m = (MonoMethod *)g_ptr_array_index (methods, i);
domain = (MonoDomain *)g_ptr_array_index (method_domains, i);
domain = user_data.domain;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need user_data.domain? can we just say domain = mono_get_root_domain(); or maybe change set_bp_in_method to call mono_get_root_domain() directly, if necessary (and I'm hoping it's not even necessary. We only really need to mention domains at the debugger protocol boundary, probably).

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the code, I think we should just delete domain from BreakpointInstance and then chase down all the places that touch that field.

seq_points = (MonoSeqPointInfo *)g_ptr_array_index (method_seq_points, i);
set_bp_in_method (domain, m, seq_points, bp, error);
}

// trying to get the seqpoints directly from the jit info of the method
// the seqpoints in get_default_jit_mm may not be found for AOTed methods in arm64
if (methods->len == 0)
if (methods->len == 0)
{
MonoJitInfo *ji;
(void)mono_jit_search_all_backends_for_jit_info (method, &ji);
Expand All @@ -464,7 +470,6 @@ mono_de_set_breakpoint (MonoMethod *method, long il_offset, EventRequest *req, M
mono_loader_unlock ();

g_ptr_array_free (methods, TRUE);
g_ptr_array_free (method_domains, TRUE);
g_ptr_array_free (method_seq_points, TRUE);

if (error && !is_ok (error)) {
Expand Down
2 changes: 2 additions & 0 deletions src/mono/mono/metadata/class.c
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
#include <mono/utils/unlocked.h>
#include <mono/utils/bsearch.h>
#include <mono/utils/checked-build.h>
// for dn_simdhash_ght_t
#include "../native/containers/dn-simdhash-specializations.h"

MonoStats mono_stats;

Expand Down
3 changes: 1 addition & 2 deletions src/mono/mono/metadata/loader-internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ struct _MonoMemoryManager {
MonoAssemblyLoadContext **alcs;

// Generic-specific caches
GHashTable *gsignature_cache;
dn_simdhash_ght_t *ginst_cache, *gmethod_cache;
dn_simdhash_ght_t *ginst_cache, *gmethod_cache, *gsignature_cache;
MonoConcurrentHashTable *gclass_cache;

/* mirror caches of ones already on MonoImage. These ones contain generics */
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/metadata/memory-manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ memory_manager_delete (MonoMemoryManager *memory_manager, gboolean debug_unload)
mono_conc_hashtable_destroy (mm->gclass_cache);
free_simdhash (&mm->ginst_cache);
free_simdhash (&mm->gmethod_cache);
free_hash (&mm->gsignature_cache);
free_simdhash (&mm->gsignature_cache);
free_hash (&mm->szarray_cache);
free_hash (&mm->array_cache);
free_hash (&mm->ptr_cache);
Expand Down
17 changes: 10 additions & 7 deletions src/mono/mono/metadata/metadata.c
Original file line number Diff line number Diff line change
Expand Up @@ -2452,7 +2452,7 @@ mono_metadata_signature_dup_internal (MonoImage *image, MonoMemPool *mp, MonoMem

/**
* Allocates memory for a MonoMethodSignature based on the provided parameters.
*
*
* @param image MonoImage for allocation.
* @param mp MonoMemPool for allocation.
* @param mem_manager MonoMemoryManager for allocation.
Expand Down Expand Up @@ -2568,10 +2568,10 @@ mono_metadata_signature_dup_delegate_invoke_to_target (MonoMethodSignature *sig)
* @param sig The original method signature.
* @param num_params The number parameters in the new signature.
* @param new_params An array of MonoType pointers representing the new parameters.
*
*
* Duplicate an existing \c MonoMethodSignature but with a new set of parameters.
* This is a Mono runtime internal function.
*
*
* @return the new \c MonoMethodSignature structure.
*/
MonoMethodSignature*
Expand Down Expand Up @@ -3339,7 +3339,7 @@ MonoMethodSignature *
mono_metadata_get_inflated_signature (MonoMethodSignature *sig, MonoGenericContext *context)
{
MonoInflatedMethodSignature helper;
MonoInflatedMethodSignature *res;
MonoInflatedMethodSignature *res = NULL;
CollectData data;

helper.sig = sig;
Expand All @@ -3354,16 +3354,19 @@ mono_metadata_get_inflated_signature (MonoMethodSignature *sig, MonoGenericConte
mono_mem_manager_lock (mm);

if (!mm->gsignature_cache)
mm->gsignature_cache = g_hash_table_new_full (inflated_signature_hash, inflated_signature_equal, NULL, (GDestroyNotify)free_inflated_signature);
// FIXME: Pick a better pre-reserved size
mm->gsignature_cache = dn_simdhash_ght_new_full (inflated_signature_hash, inflated_signature_equal, NULL, (GDestroyNotify)free_inflated_signature, 256, NULL);

// FIXME: The lookup is done on the newly allocated sig so it always fails
res = (MonoInflatedMethodSignature *)g_hash_table_lookup (mm->gsignature_cache, &helper);
dn_simdhash_ght_try_get_value (mm->gsignature_cache, &helper, (gpointer *)&res);
if (!res) {
res = mono_mem_manager_alloc0 (mm, sizeof (MonoInflatedMethodSignature));
// FIXME: sig is an inflated signature not owned by the mem manager
res->sig = sig;
res->context.class_inst = context->class_inst;
res->context.method_inst = context->method_inst;
g_hash_table_insert (mm->gsignature_cache, res, res);
// FIXME: We're wasting memory and cpu by storing key and value redundantly for this table
dn_simdhash_ght_insert (mm->gsignature_cache, res, res);
}

mono_mem_manager_unlock (mm);
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/mini/aot-runtime.c
Original file line number Diff line number Diff line change
Expand Up @@ -3417,8 +3417,8 @@ decode_exception_debug_info (MonoAotModule *amodule,
jit_mm_lock (jit_mm);
/* This could be set already since this function can be called more than once for the same method */
MonoSeqPointInfo *existing_seq_points = NULL;
if (!g_hash_table_lookup_extended (jit_mm->seq_points, method, NULL, (gpointer *)&existing_seq_points)) {
g_hash_table_insert (jit_mm->seq_points, method, seq_points);
if (!dn_simdhash_ght_try_get_value (jit_mm->seq_points, method, (void **)&existing_seq_points)) {
dn_simdhash_ght_insert (jit_mm->seq_points, method, seq_points);
} else {
mono_seq_point_info_free (seq_points);
seq_points = existing_seq_points;
Expand Down
25 changes: 16 additions & 9 deletions src/mono/mono/mini/interp/tiering.c
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
#include "tiering.h"

static mono_mutex_t tiering_mutex;
static GHashTable *patch_sites_table;
// FIXME: The add/remove traffic on this table may require dn_simdhash to implement cascade flag cleanup
// and compaction
static dn_simdhash_ptr_ptr_t *patch_sites_table;
static gboolean enable_tiering;

void
mono_interp_tiering_init (void)
{
mono_os_mutex_init_recursive (&tiering_mutex);
patch_sites_table = g_hash_table_new (NULL, NULL);
patch_sites_table = dn_simdhash_ptr_ptr_new (0, NULL);
enable_tiering = TRUE;
}

Expand Down Expand Up @@ -61,11 +63,12 @@ patch_imethod_site (gpointer data, gpointer user_data)
static void
patch_interp_data_items (InterpMethod *old_imethod, InterpMethod *new_imethod)
{
GSList *sites = g_hash_table_lookup (patch_sites_table, old_imethod);
g_slist_foreach (sites, patch_imethod_site, new_imethod);

g_hash_table_remove (patch_sites_table, old_imethod);
g_slist_free (sites);
GSList *sites = NULL;
if (dn_simdhash_ptr_ptr_try_get_value (patch_sites_table, old_imethod, (void **)&sites)) {
g_slist_foreach (sites, patch_imethod_site, new_imethod);
dn_simdhash_ptr_ptr_try_remove (patch_sites_table, old_imethod);
g_slist_free (sites);
}
}

static InterpMethod*
Expand Down Expand Up @@ -100,9 +103,13 @@ static void
register_imethod_patch_site (InterpMethod *imethod, gpointer *ptr)
{
g_assert (!imethod->optimized);
GSList *sites = g_hash_table_lookup (patch_sites_table, imethod);
GSList *sites = NULL;
guint8 found = dn_simdhash_ptr_ptr_try_get_value (patch_sites_table, imethod, (void **)&sites);
sites = g_slist_prepend (sites, ptr);
g_hash_table_insert_replace (patch_sites_table, imethod, sites, TRUE);
if (found)
dn_simdhash_ptr_ptr_try_replace_value (patch_sites_table, imethod, sites);
else
dn_simdhash_ptr_ptr_try_add (patch_sites_table, imethod, sites);
}

static void
Expand Down
31 changes: 19 additions & 12 deletions src/mono/mono/mini/interp/transform.c
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,8 @@ init_last_ins_call (TransformData *td)
static guint32
get_data_item_wide_index (TransformData *td, void *ptr, gboolean *new_slot)
{
gpointer p = g_hash_table_lookup (td->data_hash, ptr);
gpointer p = NULL;
guint8 found = dn_simdhash_ptr_ptr_try_get_value (td->data_hash, ptr, (void **)&p);
guint32 index;
if (p != NULL) {
if (new_slot)
Expand All @@ -1160,7 +1161,10 @@ get_data_item_wide_index (TransformData *td, void *ptr, gboolean *new_slot)
index = td->n_data_items;
td->data_items [index] = ptr;
++td->n_data_items;
g_hash_table_insert (td->data_hash, ptr, GUINT_TO_POINTER (index + 1));
if (found)
dn_simdhash_ptr_ptr_try_replace_value (td->data_hash, ptr, GUINT_TO_POINTER (index + 1));
else
dn_simdhash_ptr_ptr_try_add (td->data_hash, ptr, GUINT_TO_POINTER (index + 1));
if (new_slot)
*new_slot = TRUE;
return index;
Expand Down Expand Up @@ -2998,7 +3002,7 @@ interp_inline_method (TransformData *td, MonoMethod *target_method, MonoMethodHe

/* Remove any newly added items */
for (i = prev_n_data_items; i < td->n_data_items; i++) {
g_hash_table_remove (td->data_hash, td->data_items [i]);
dn_simdhash_ptr_ptr_try_remove (td->data_hash, td->data_items [i]);
}
td->n_data_items = prev_n_data_items;
/* Also remove any added indexes from the imethod list */
Expand Down Expand Up @@ -3969,7 +3973,8 @@ interp_transform_call (TransformData *td, MonoMethod *method, MonoMethod *target
if (MINT_IS_PATCHABLE_CALL (td->last_ins->opcode)) {
g_assert (!calli && !is_virtual);
td->last_ins->flags |= INTERP_INST_FLAG_RECORD_CALL_PATCH;
g_hash_table_insert (td->patchsite_hash, td->last_ins, target_method);
if (!dn_simdhash_ptr_ptr_try_add (td->patchsite_hash, td->last_ins, target_method))
dn_simdhash_ptr_ptr_try_replace (td->patchsite_hash, td->last_ins, target_method);
}
#endif
}
Expand Down Expand Up @@ -8983,9 +8988,10 @@ emit_compacted_instruction (TransformData *td, guint16* start_ip, InterpInst *in
g_assert (MINT_IS_PATCHABLE_CALL (opcode));

/* TODO: could `ins` be removed by any interp optimization? */
MonoMethod *target_method = (MonoMethod *) g_hash_table_lookup (td->patchsite_hash, ins);
MonoMethod *target_method = NULL;
dn_simdhash_ptr_ptr_try_get_value (td->patchsite_hash, ins, (void **)&target_method);
g_assert (target_method);
g_hash_table_remove (td->patchsite_hash, ins);
dn_simdhash_ptr_ptr_try_remove (td->patchsite_hash, ins);

mini_tiered_record_callsite (start_ip, target_method, TIERED_PATCH_KIND_INTERP);

Expand Down Expand Up @@ -9275,9 +9281,9 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG
td->dummy_var = -1;
td->ref_handle_var = -1;
td->data_items = NULL;
td->data_hash = g_hash_table_new (NULL, NULL);
td->data_hash = dn_simdhash_ptr_ptr_new (0, NULL);
#ifdef ENABLE_EXPERIMENT_TIERED
td->patchsite_hash = g_hash_table_new (NULL, NULL);
td->patchsite_hash = dn_simdhash_ptr_ptr_new (0, NULL);
#endif
td->gen_seq_points = !mini_debug_options.no_seq_points_compact_data || mini_debug_options.gen_sdb_seq_points;
td->gen_sdb_seq_points = mini_debug_options.gen_sdb_seq_points;
Expand Down Expand Up @@ -9498,9 +9504,9 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG
g_free (td->renamable_vars);
g_free (td->renamed_fixed_vars);
g_free (td->local_ref_count);
g_hash_table_destroy (td->data_hash);
dn_simdhash_free (td->data_hash);
#ifdef ENABLE_EXPERIMENT_TIERED
g_hash_table_destroy (td->patchsite_hash);
dn_simdhash_free (td->patchsite_hash);
#endif
g_ptr_array_free (td->seq_points, TRUE);
if (td->line_numbers)
Expand Down Expand Up @@ -9681,9 +9687,10 @@ mono_interp_transform_method (InterpMethod *imethod, ThreadContext *context, Mon

// FIXME Publishing of seq points seems to be racy with tiereing. We can have both tiered and untiered method
// running at the same time. We could therefore get the optimized imethod seq points for the unoptimized method.
gpointer seq_points = g_hash_table_lookup (jit_mm->seq_points, imethod->method);
gpointer seq_points = NULL;
dn_simdhash_ght_try_get_value (jit_mm->seq_points, imethod->method, (void **)&seq_points);
if (!seq_points || seq_points != imethod->jinfo->seq_points)
g_hash_table_replace (jit_mm->seq_points, imethod->method, imethod->jinfo->seq_points);
dn_simdhash_ght_replace (jit_mm->seq_points, imethod->method, imethod->jinfo->seq_points);
}
jit_mm_unlock (jit_mm);

Expand Down
8 changes: 5 additions & 3 deletions src/mono/mono/mini/interp/transform.h
Original file line number Diff line number Diff line change
Expand Up @@ -311,10 +311,12 @@ typedef struct
int n_data_items;
int max_data_items;
void **data_items;
GHashTable *data_hash;
// FIXME: ptr_u32
dn_simdhash_ptr_ptr_t *data_hash;
GSList *imethod_items;
#ifdef ENABLE_EXPERIMENT_TIERED
GHashTable *patchsite_hash;
// FIXME: ptr_u32
dn_simdhash_ptr_ptr_t *patchsite_hash;
#endif
int *clause_indexes;
int *clause_vars;
Expand Down Expand Up @@ -504,7 +506,7 @@ interp_dump_ins (InterpInst *ins, gpointer *data_items);
InterpInst*
interp_get_ldc_i4_from_const (TransformData *td, InterpInst *ins, gint32 ct, int dreg);

gint32
gint32
interp_get_const_from_ldc_i4 (InterpInst *ins);

int
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/mini/interp/whitebox.c
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ transform_method (MonoDomain *domain, MonoImage *image, TestItem *ti)
td->rtm = rtm;
td->clause_indexes = (int*)g_malloc (header->code_size * sizeof (int));
td->data_items = NULL;
td->data_hash = g_hash_table_new (NULL, NULL);
td->data_hash = dn_simdhash_ptr_ptr_new (0, NULL);
/* TODO: init more fields of `td` */

mono_test_interp_method_compute_offsets (td, rtm, signature, header);
Expand Down
Loading
Loading