[hot_reload] Implement table lookup of modified rows

Add a bool array on the base image to keep track of whether each table had any
row modifications (as opposed to row additions) in any generation.

If there was a modification, take the slow path in mono_image_effective_table
even if the index we're looking at is in the base image.

Update hot_reload_effective_table_slow so that if there was a modified row, we
look at all the deltas to see if there's an even later update to that
row.  (When there are only additions, keep same behavior as before - only look
as far as the generation that added the row we wanted to find).

Also refine the assertion in hot_reload_relative_delta_index to properly
account for EnCMap entries that correspond to modifications - in that case we
might stop searching a metadata delta before we hit the end of the table if the
EnCmap entries start pointing to rows that are past the one we wanted to look
up.

src/mono/mono/component/hot_reload-stub.c

@@ -59,6 +59,9 @@ hot_reload_stub_table_bounds_check (MonoImage *base_image, int table_index, int
 static gboolean
 hot_reload_stub_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc get_heap, uint32_t orig_index, MonoImage **image_out, uint32_t *index_out);
 
+static gboolean
+hot_reload_stub_has_modified_rows (const MonoTableInfo *table);
+
 static MonoComponentHotReload fn_table = {
 	{ MONO_COMPONENT_ITF_VERSION, &hot_reload_stub_available },
 	&hot_reload_stub_set_fastpath_data,
@@ -75,6 +78,7 @@ static MonoComponentHotReload fn_table = {
 	&hot_reload_stub_get_updated_method_rva,
 	&hot_reload_stub_table_bounds_check,
 	&hot_reload_stub_delta_heap_lookup,
+	&hot_reload_stub_has_modified_rows,
 };
 
 static bool
@@ -172,6 +176,11 @@ hot_reload_stub_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc
 	g_assert_not_reached ();
 }
 
+static gboolean
+hot_reload_stub_has_modified_rows (const MonoTableInfo *table){
+	return FALSE;
+}
+
 MONO_COMPONENT_EXPORT_ENTRYPOINT
 MonoComponentHotReload *
 mono_component_hot_reload_init (void)

src/mono/mono/component/hot_reload.c

@@ -73,6 +73,9 @@ hot_reload_table_bounds_check (MonoImage *base_image, int table_index, int token
 static gboolean
 hot_reload_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc get_heap, uint32_t orig_index, MonoImage **image_out, uint32_t *index_out);
 
+static gboolean
+hot_reload_has_modified_rows (const MonoTableInfo *table);
+
 static MonoComponentHotReload fn_table = {
 	{ MONO_COMPONENT_ITF_VERSION, &hot_reload_available },
 	&hot_reload_set_fastpath_data,
@@ -89,6 +92,7 @@ static MonoComponentHotReload fn_table = {
 	&hot_reload_get_updated_method_rva,
 	&hot_reload_table_bounds_check,
 	&hot_reload_delta_heap_lookup,
+	&hot_reload_has_modified_rows,
 };
 
 MonoComponentHotReload *
@@ -161,6 +165,9 @@ typedef struct _BaselineInfo {
 
 	/* Maps MethodDef token indices to a boolean flag that there's an update for the method */
 	GHashTable *method_table_update;
+
+	/* TRUE if any published update modified an existing row */
+	gboolean any_modified_rows [MONO_TABLE_NUM];
 } BaselineInfo;
 
 #define DOTNET_MODIFIABLE_ASSEMBLIES "DOTNET_MODIFIABLE_ASSEMBLIES"
@@ -724,9 +731,6 @@ dump_update_summary (MonoImage *image_base, MonoImage *image_dmeta)
 void
 hot_reload_effective_table_slow (const MonoTableInfo **t, int *idx)
 {
-	if (G_LIKELY (*idx < table_info_get_rows (*t)))
-		return;
-
 	/* FIXME: don't let any thread other than the updater thread see values from a delta image
 	 * with a generation past update_published
 	 */
@@ -738,17 +742,22 @@ hot_reload_effective_table_slow (const MonoTableInfo **t, int *idx)
 	if (!info)
 		return;
 
-	GList *list = info->delta_image;
-	MonoImage *dmeta;
-	int ridx;
-	MonoTableInfo *table;
-
 	/* Invariant: `*t` must be a `MonoTableInfo` of the base image. */
 	g_assert (base->tables < *t && *t < &base->tables [MONO_TABLE_LAST]);
 
 	size_t s = ALIGN_TO (sizeof (MonoTableInfo), sizeof (gpointer));
 	int tbl_index = ((intptr_t) *t - (intptr_t) base->tables) / s;
 
+	if (G_LIKELY (*idx < table_info_get_rows (*t) && !info->any_modified_rows[tbl_index]))
+		return;
+
+	gboolean any_modified = info->any_modified_rows[tbl_index];
+
+	GList *list = info->delta_image;
+	MonoImage *dmeta;
+	int ridx;
+	MonoTableInfo *table;
+
 	/* FIXME: I don't understand how ReplaceMethodOften works - it always has a 
 	 * EnCMap  entry 2: 0x06000002 (MethodDef) for every revision.	Shouldn't the number of methodDef rows be going up?
 
@@ -772,19 +781,43 @@ hot_reload_effective_table_slow (const MonoTableInfo **t, int *idx)
 	 */
 	int g = 0;
 
+	/* Candidate: the last delta that had updates for the requested row */
+	MonoImage *cand_dmeta = NULL;
+	MonoTableInfo *cand_table = NULL;
+	int cand_ridx = -1;
+	int cand_g = 0;
+
+	gboolean cont;
 	do {
 		g_assertf (list, "couldn't find idx=0x%08x in assembly=%s", *idx, dmeta && dmeta->name ? dmeta->name : "unknown image");
 		dmeta = (MonoImage*)list->data;
 		list = list->next;
 		table = &dmeta->tables [tbl_index];
-		ridx = hot_reload_relative_delta_index (dmeta, mono_metadata_make_token (tbl_index, *idx + 1)) - 1;
+		int rel_row = hot_reload_relative_delta_index (dmeta, mono_metadata_make_token (tbl_index, *idx + 1));
+		g_assert (rel_row == -1 || (rel_row > 0 && rel_row <= table_info_get_rows (table)));
 		g++;
-	} while (ridx < 0 || ridx >= table_info_get_rows (table));
+		if (rel_row != -1) {
+			cand_dmeta = dmeta;
+			cand_table = table;
+			cand_ridx = rel_row - 1;
+			cand_g = g;
+		}
+		ridx = rel_row - 1;
+		if (!any_modified) {
+			/* if the table only got additions, not modifications, don't continue after we find the first image that has the right number of rows */
+			cont = ridx < 0 || ridx >= table_info_get_rows (table);
+		} else {
+			/* otherwise, keep going in case a later generation modified the row again */
+			cont = list != NULL;
+		}
+	} while (cont);
 
-	mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "effective table for %s: 0x%08x -> 0x%08x (rows = 0x%08x) (gen %d, g %d)", mono_meta_table_name (tbl_index), *idx, ridx, table_info_get_rows (table), metadata_update_local_generation (base, info, dmeta), g);
+	if (cand_ridx != -1) {
+		mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "effective table for %s: 0x%08x -> 0x%08x (rows = 0x%08x) (gen %d, g %d)", mono_meta_table_name (tbl_index), *idx, cand_ridx, table_info_get_rows (cand_table), metadata_update_local_generation (base, info, cand_dmeta), cand_g);
 
-	*t = table;
-	*idx = ridx;
+		*t = cand_table;
+		*idx = cand_ridx;
+	}
 }
 
 /*
@@ -835,6 +868,11 @@ hot_reload_relative_delta_index (MonoImage *image_dmeta, int token)
 	mono_metadata_decode_row (encmap, index_map - 1, cols, MONO_ENCMAP_SIZE);
 	int map_entry = cols [MONO_ENCMAP_TOKEN];
 
+	/* we're looking at the beginning of a sequence of encmap rows that are all the
+	 * modifications+additions for the table we are looking for (or we're looking at an entry
+	 * for the next table after the one we wanted).  the map entries will have tokens in
+	 * increasing order.  skip over the rows where the tokens are not the one we want, until we
+	 * hit the rows for the next table or we hit the end of the encmap */
 	while (mono_metadata_token_table (map_entry) == table && mono_metadata_token_index (map_entry) < index && index_map < encmap_rows) {
 		mono_metadata_decode_row (encmap, ++index_map - 1, cols, MONO_ENCMAP_SIZE);
 		map_entry = cols [MONO_ENCMAP_TOKEN];
@@ -848,12 +886,18 @@ hot_reload_relative_delta_index (MonoImage *image_dmeta, int token)
 			mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "relative index for token 0x%08x -> table 0x%02x row 0x%08x", token, table, return_val);
 			return return_val;
 		} else {
-			/* otherwise the last entry in the encmap is for this table, but is still less than the index - the index is in the next generation */
-			g_assert (mono_metadata_token_index (map_entry) < index && index_map == encmap_rows);
+			/* Otherwise we stopped either: because we saw an an entry for a row after
+			 * the one we wanted - we were looking for a modification, but the encmap
+			 * has an addition; or, because we saw the last entry in the encmap and it
+			 * still wasn't for a row as high as the one we wanted.  either way, the
+			 * update we want is not in the delta we're looking at.
+			 */
+			g_assert ((mono_metadata_token_index (map_entry) > index) || (mono_metadata_token_index (map_entry) < index && index_map == encmap_rows));
 			return -1;
 		}
 	} else {
-		/* otherwise there are no more encmap entries for this table, and we didn't see the index, so there index is in the next generation */
+		/* otherwise there are no more encmap entries for this table, and we didn't see the
+		 * index, so there was no modification/addition for that index in this delta. */
 		g_assert (mono_metadata_token_table (map_entry) > table);
 		return -1;
 	}
@@ -925,9 +969,10 @@ delta_info_compute_table_records (MonoImage *image_dmeta, MonoImage *image_base,
 		g_assert (table != MONO_TABLE_ENCMAP);
 		g_assert (table >= prev_table);
 		/* FIXME: check bounds - is it < or <=. */
-		if (rid < delta_info->count[table].prev_gen_rows)
+		if (rid < delta_info->count[table].prev_gen_rows) {
+			base_info->any_modified_rows[table] = TRUE;
 			delta_info->count[table].modified_rows++;
-		else
+		} else
 			delta_info->count[table].inserted_rows++;
 		if (table == prev_table)
 			continue;
@@ -1512,3 +1557,22 @@ hot_reload_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc get_
 	return (cur != NULL);
 }
 
+static gboolean
+hot_reload_has_modified_rows (const MonoTableInfo *table)
+{
+	MonoImage *base = table_info_get_base_image (table);
+	if (!base)
+		return FALSE;
+	BaselineInfo *info = baseline_info_lookup (base);
+	if (!info)
+		return FALSE;
+
+	/* Invariant: `*t` must be a `MonoTableInfo` of the base image. */
+	g_assert (base->tables < table && table < &base->tables [MONO_TABLE_LAST]);
+
+	size_t s = ALIGN_TO (sizeof (MonoTableInfo), sizeof (gpointer));
+	int tbl_index = ((intptr_t) table - (intptr_t) base->tables) / s;
+
+	return info->any_modified_rows[tbl_index];
+}
+

src/mono/mono/component/hot_reload.h

@@ -29,6 +29,8 @@ typedef struct _MonoComponentHotReload {
 	gpointer (*get_updated_method_rva) (MonoImage *base_image, uint32_t idx);
 	gboolean (*table_bounds_check) (MonoImage *base_image, int table_index, int token_index);
 	gboolean (*delta_heap_lookup) (MonoImage *base_image, MetadataHeapGetterFunc get_heap, uint32_t orig_index, MonoImage **image_out, uint32_t *index_out);
+	gboolean (*has_modified_rows) (const MonoTableInfo *table);
+
 } MonoComponentHotReload;
 
 MONO_COMPONENT_EXPORT_ENTRYPOINT

src/mono/mono/metadata/metadata-internals.h

@@ -805,11 +805,14 @@ mono_metadata_has_updates (void)
 void
 mono_image_effective_table_slow (const MonoTableInfo **t, int *idx);
 
+gboolean
+mono_metadata_update_has_modified_rows (const MonoTableInfo *t);
+
 static inline void
 mono_image_effective_table (const MonoTableInfo **t, int *idx)
 {
 	if (G_UNLIKELY (mono_metadata_has_updates ())) {
-		if (G_UNLIKELY (*idx >= table_info_get_rows ((*t)))) {
+		if (G_UNLIKELY (*idx >= table_info_get_rows ((*t)) || mono_metadata_update_has_modified_rows (*t))) {
 			mono_image_effective_table_slow (t, idx);
 		}
 	}

src/mono/mono/metadata/metadata-update.c

@@ -121,3 +121,8 @@ mono_metadata_update_delta_heap_lookup (MonoImage *base_image, MetadataHeapGette
 }
 
 
+gboolean
+mono_metadata_update_has_modified_rows (const MonoTableInfo *table)
+{
+	return mono_component_hot_reload ()->has_modified_rows (table);
+}