[Mono-dev] Ping on nternal call builders

[Kornél Pál]

Hi,

> Rodrigo Kumpera wrote:
>  From your email the advantage of the patch is that by replacing some icalls
> with managed code we would get rid of some managed-native transitions.

The long term goal would be to replace all native icalls with managed
icalls (build by icall builders) that call no native code that could
eliminate all unnecessary managed to native transitions.

> -It makes the new full AOT mode a bit more complicated as these would be 
> another
> kind of wrappers that would need to be AOT'ed.

The current string constructor hach is implemented using
managed-to-managed wrappers. If that works, there should be no problem
with AOT. But I don't use AOT so I have little experience with it.

> -Increases the footprint by just a bit, as we need to store the icalls 
> internal representation
> as well. The amount of extra code generated is bounded, specially since 
> we replace a
> managed-to-native wrapper with a managed-to-managed with more code. 

All the three reference implementations implement very small and fast
code blocks and I believe that it's worth to aviod managed to native
transitions. OffsetToStringData is an exception because that is managed
already but I wanted to show that mini_get_inst_for_method could be
replaced by icall builders.

> -JIT time should not change much, but this is only a guess.

JIT time should be the same as for IL code contained in assemblies. Lots
of our current icalls are native because we couldn't implement them in
C#. Other icalls are native calls because they call external native code
so it's faster to have only one managed to native transition. I don't
think that there are icalls that are in native code because have better
performance than an IL implementation.

> I wonder if it is really an advantage of following this patch as, for 
> example, OffsetToStringData
> has 22 lines in your patch but 6 in the current setup. Of course the 
> code in your patch ends more
> organized and I find it easier to follow, but should be interesting to 
> rear from the others.

The 22 lines are the builder but that gets executed only once. After
that only the generated (and JITed) code is used. The actual IL code
just returns a constant.

> -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.)

Kornél

> 2008/7/29 Kornél Pál <kornelpal at gmail.com <mailto:kornelpal at gmail.com>>
> 
>     Hi,
> 
>     There is a pending patch:
>     http://lists.ximian.com/pipermail/mono-devel-list/2008-June/028291.html
> 
>     As far as I can remember I didn't get any feedback regarding this patch.
> 
>     Please review the patch. Thanks.
> 
>     Kornél
>     _______________________________________________
>     Mono-devel-list mailing list
>     Mono-devel-list at lists.ximian.com
>     <mailto:Mono-devel-list at lists.ximian.com>
>     http://lists.ximian.com/mailman/listinfo/mono-devel-list
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ICallBuilderPerfTest.cs
Url: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20080730/ab3f64c0/attachment-0001.pl 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: icall_builder.diff.txt
Url: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20080730/ab3f64c0/attachment-0001.txt