[Mono-dev] [PATCH] MONO_IOMAP reporting option - string allocation locations

[Rodrigo Kumpera]

On Fri, Nov 27, 2009 at 4:43 PM, Marek Habersack <grendel at twistedcode.net>wrote:

> On 11/27/2009 06:48 PM, Rodrigo Kumpera wrote:
>
>>
>>
>> On Fri, Nov 27, 2009 at 1:40 PM, Marek Habersack
>> <grendel at twistedcode.net <mailto:grendel at twistedcode.net>> wrote:
>>
>>    On 11/27/2009 02:29 PM, Rodrigo Kumpera wrote:
>>
>>        I agree with Zoltan, we better figure out how to extend the
>>        profilling
>>        interface to support such tool than
>>
>>    it would defy the purpose of the tool, but it seems I can remove the
>>    code from mono_string_new_utf16 without harming functionality -
>>    would that be ok?
>>
>>
>> Why using the profilling interface defy the purpose of the tool? I can't
>> see how passing an extra argument to the mono runtime be a problem.
>>
> The utility should be as seamless to use. With it consisting of two parts -
> one in IO portability code and another as a profiler module ('profiler'
> being a misnomer here too, as the tool is by no means a profiler) it would
> require setting both MONO_IOMAP and passing --profile=iomap (or somesuch) to
> the runtime as they both are required for the code to be useful. This can be
> a problem for MonoVS or MonoDevelop, which would have to call the runtime
> with --profile for every app whether or not it is necessary, and also it
> introduces a complexity for users not familiar with Mono command line. To
> get rid of the disparity, one would have to switch IOMAP on behind the
> scenes if --profile=iomap was used and this is not a nice thing to do, imho.
> From the other end, the user would have to know that they need to specify
> 'all:report' and pass --profile=iomap to the runtime - also not a nice
> solution.


Ok, how about doing it just like sdb?  Using the profiler API, but from the
runtime itself? We could ship the code in IOMAP and MONO_IOMAP=


 just fine, I don't see a reason to add it to the runtime.
> As I mentioned, we can extend the profiling API to fit your needs.
>
The profiling API has all that's needed - but if the code was there, it
> would have to hook up to the object allocation function and examine every
> single object whether it's a string and then store the string - it would
> cost more time it does with the current patch.
>

Well, you just mentioned what you need from the profiler, a way to tell the
runtime that all you care about is string allocation.


Now, a few bits about your patch:

The whole mono_jit_stack_backtrace thing is not required, you can get the
same results with minimal perf hit by using mono_stack_walk_no_il.
This is true since mono_jit_stack_backtrace and mono_jit_walk_stack_from_ctx
are basically identical. All you need to do is return TRUE from
the user func once you got enough frames.



--- a/mono/utils/mono-io-portability.c
+++ b/mono/utils/mono-io-portability.c

@@ -130,6 +190,13 @@ void mono_portability_helpers_init (void)
         InitializeCriticalSection (&mismatched_files_section);
         MONO_GC_REGISTER_ROOT (mismatched_files_hash);
         mismatched_files_hash = mono_g_hash_table_new
(mismatched_files_guint32_hash, mismatched_files_guint32_equal);
+
+        MONO_GC_REGISTER_ROOT (saved_strings_hash);
+        saved_strings_hash = mono_g_hash_table_new (NULL, NULL);
+
+        MONO_GC_REGISTER_ROOT (string_locations_hash);
+        string_locations_hash = mono_g_hash_table_new
(mismatched_files_guint32_hash, mismatched_files_guint32_equal);

Using MONO_GC_REGISTER_ROOT here won't really work. It might work with
boehm, but that's far from optimal.

Since you use string as key, you can use  mono_g_hash_table_new_type with
MONO_HASH_CONSERVATIVE_GC
since the value has an embedded MonoString in it.


+
+static gboolean ignore_frame (MonoMethod *method)
+{

You can replaces all those types comparisons with "klass->image ==
mono_defaults.corlib" since
you discard corlib types anyway.


+    EnterCriticalSection (&mismatched_files_section);
+    head = (SavedString*)mono_g_hash_table_lookup (saved_strings_hash,
(gpointer)str);

I just noticed you construct the hashtable without hash or equal functions,
this means keys will be compared by their pointer value
and not content. Is that as intended? Because it makes the whole chaining
you do here pretty useless as no 2 strings will be equal.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20091127/30793e33/attachment.html