<br><br><div class="gmail_quote">On Fri, Nov 27, 2009 at 4:43 PM, Marek Habersack <span dir="ltr">&lt;<a href="mailto:grendel@twistedcode.net">grendel@twistedcode.net</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">On 11/27/2009 06:48 PM, Rodrigo Kumpera wrote:<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="im">
<br>
<br>
On Fri, Nov 27, 2009 at 1:40 PM, Marek Habersack<br></div><div class="im">
&lt;<a href="mailto:grendel@twistedcode.net" target="_blank">grendel@twistedcode.net</a> &lt;mailto:<a href="mailto:grendel@twistedcode.net" target="_blank">grendel@twistedcode.net</a>&gt;&gt; wrote:<br>
<br>
    On 11/27/2009 02:29 PM, Rodrigo Kumpera wrote:<br>
<br>
        I agree with Zoltan, we better figure out how to extend the<br>
        profilling<br>
        interface to support such tool than<br>
<br>
    it would defy the purpose of the tool, but it seems I can remove the<br>
    code from mono_string_new_utf16 without harming functionality -<br>
    would that be ok?<br>
<br>
<br>
Why using the profilling interface defy the purpose of the tool? I can&#39;t<br>
see how passing an extra argument to the mono runtime be a problem.<br>
</div></blockquote>
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 (&#39;profiler&#39; 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 &#39;all:report&#39; and pass --profile=iomap to the runtime - also not a nice solution.</blockquote>
<div><br>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=<br></div><div class="im"><br><br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
just fine, I don&#39;t see a reason to add it to the runtime.<br>
As I mentioned, we can extend the profiling API to fit your needs.<br>
</blockquote></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
The profiling API has all that&#39;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&#39;s a string and then store the string - it would cost more time it does with the current patch.<br>
</blockquote><div><br>Well, you just mentioned what you need from the profiler, a way to tell the runtime that all you care about is string allocation.<br><br><br>Now, a few bits about your patch:<br><br>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.<br>
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<br>the user func once you got enough frames.<br><br><br><br>--- a/mono/utils/mono-io-portability.c<br>
+++ b/mono/utils/mono-io-portability.c<br><br>@@ -130,6 +190,13 @@ void mono_portability_helpers_init (void)<br>         InitializeCriticalSection (&amp;mismatched_files_section);<br>         MONO_GC_REGISTER_ROOT (mismatched_files_hash);<br>
         mismatched_files_hash = mono_g_hash_table_new (mismatched_files_guint32_hash, mismatched_files_guint32_equal);<br>+<br>+        MONO_GC_REGISTER_ROOT (saved_strings_hash);<br>+        saved_strings_hash = mono_g_hash_table_new (NULL, NULL);<br>
+<br>+        MONO_GC_REGISTER_ROOT (string_locations_hash);<br>+        string_locations_hash = mono_g_hash_table_new (mismatched_files_guint32_hash, mismatched_files_guint32_equal);<br><br>Using MONO_GC_REGISTER_ROOT here won&#39;t really work. It might work with boehm, but that&#39;s far from optimal.<br>
<br>Since you use string as key, you can use  mono_g_hash_table_new_type with MONO_HASH_CONSERVATIVE_GC<br>since the value has an embedded MonoString in it.<br><br><br>+<br>+static gboolean ignore_frame (MonoMethod *method)<br>
+{<br><br>You can replaces all those types comparisons with &quot;klass-&gt;image == mono_defaults.corlib&quot; since<br>you discard corlib types anyway.<br><br><br>+    EnterCriticalSection (&amp;mismatched_files_section);<br>
+    head = (SavedString*)mono_g_hash_table_lookup (saved_strings_hash, (gpointer)str);<br><br>I just noticed you construct the hashtable without hash or equal functions, this means keys will be compared by their pointer value<br>
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.<br><br><br></div></div>