[Mono-dev] Ping on nternal call builders

[Rodrigo Kumpera]

On Tue, Jul 29, 2008 at 7:35 PM, Kornél Pál <kornelpal at gmail.com> wrote:

 -For simple methods such as OffsetToStringData the method call overhead
> might be a killer. We
> should make sure that these new wrappers get a change to be inlined.
>

Currently it doesn't get inlined because the JIT has some logic
> preventing (at least types) of wrappers being inlined but should be
> possible to inline them.
>
>  -I'm not aware of how stack traces behave with wrappers, but are they
>> preserved with your patch?
>>
>
> As long as they are not inlined the stack trace is preserved. But this
> is true for Class Library methods implemented in C# so icall builders
> should not be treated differently.
>
>  -Does it work under trunk?
>>
>
> It should but the patch may be outdated.
>
>  -Do you have performance numbers on your change? Since it changes
>> performance sensitive
>> parts of the runtime, attaching a benchmark (or pointing to an existing
>> one) showing the implications is
>> fundamental.
>>
>
> These are only reference implementations and OffsetToStringData for
> example is slower for sure because it don't get inlined. I expect string
> .ctors to have the same performance because the generated IL code is the
> same but using icall builder is a clean way as opposed to the current hack.
> UnsafeAddrOfPinnedArrayElement should benefit because no native code will be
> called.
>
> As for OffsetToStringData I don't know whether mini_get_inst_for_method or
> an icall builder is faster when generating the code but using an icall
> builder solves the things at the highest possible level letting low level
> optimizations do their job.
>
> I don't insist on getting these reference icall builder implementations
> into trunk I just would like to get approval for the concept and the
> support code of icall builders. Actual icall builder implementations could
> be evaluated independently.
>
> With the attched test I got:
> mono r109162: 99060
> mono r109162 with my patch: 17375 (5.7x faster)
> MS.NET 2.0 SP1: 17898
>
> The imporvement is purely because of the method being implemented in
> managed code. So mono would benefit form icall builders for sure.
>
> The code that decides what to be inlined is too cryptic to me because I
> believe that there are redundant checks against wrappers but if someone
> could enable the inlining of managed-to-managed wrappers icall builders
> could provide even more performance improvement.
>

> I also have attached an updated patch. (Only line numbers were updated no
> code modification was necessary.)
>
>
Making those wrappers inlineable should be straight forward, just add a
mono_marshal_is_managed_icall that checks this info and verify for it in
mono_method_check_inlining and in the code_block that activates inlining for
CEE_CALL. I don't understand the particularities of wrappers, so it might
not be a good idea making all M2M wrapper inlinable. Other than that, it
might be interesting to have support to force it on some of then as well.

Other option is to kill the inline_info bit in MonoMethod and use the space
for a force_inline bit. inline_info can easily be replaced by a hash table
in mini.c. This would be more general and useful.

I like the idea of having some intrinsics expressed as IL instead of the
mini_get_inst_for_method mess. Maybe we should move to use this model and
just leave the icall machine as it currently is.

I think this patch is really nice, so unless someone else opposes to it we
should move forward to put it into commit shape - split the patch in a few
smaller ones: one with just the new icall machinery, other adding the
inlining behavior changes and finally another one with the converted icalls
of your patch.


Rodrigo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20080729/72973d77/attachment.html