Re: The future of Mono's profiler API

I think its mostly a matter of being able to automatically re-thunk
which is useful for other reasons as well.

On Wed, Jun 21, 2017 at 7:04 PM, Rodrigo Kumpera <[email protected]> wrote:
> Hey Greg/Alex,
>
> We should look at JVMTI Capabilities, as it was designed to handle this.
> In JVMTI, the runtime has a set of capabilities than can be enabled at
> certain phases of the application. IE, some can only be enabled during
> startup, others any time.
> Furthermore, those capabilities, once enabled, will remain latent if later
> disabled. IE, you can put gc allocation as a latent capability during
> startup and only later actually enable it.
>
>
> --
> Rodrigo
>
>
>
>
> On Tue, Jun 20, 2017 at 9:44 AM, Alex Rønne Petersen <[email protected]>
> wrote:
>>
>> (Re-sending since my last email didn't go to the list for some reason.)
>>
>> Hey Greg,
>>
>> One possibility is that we could use a new entry point name for the
>> new version of the profiler API. That way, if we detect that a
>> profiler module has the old entry point name, we could print an error
>> and refuse to load it, rather than relying on the dynamic linker to
>> throw errors when mono_profiler_install_* functions are invoked by the
>> profiler modules, Does this sound reasonable?
>>
>> Regarding dynamic enter/leave hooking, I agree that it would be a
>> super cool feature to have. Unfortunately, it would require a
>> significant amount of work on the JIT side as re-JITing code is a hard
>> problem to solve reliably on most architectures. There are other
>> reasons I could see re-JITing being a useful feature to have (e.g.
>> incremental optimization based on profiling), but I can't really say
>> definitively whether we'll ever do it.
>>
>> Regards,
>> Alex
>>
>> On Tue, Jun 20, 2017 at 11:04 AM, Greg Young <[email protected]>
>> wrote:
>> > So the possible issue with option #2 that I see is in distribution for
>> > 3rd party profilers like privateeye. I don't see this as a huge issue
>> > but it might be useful to at least be able to load the old API still
>> > (not work) so the old version of the profiler could realize it is on a
>> > newer version and exit (or the runtime could recognize this and give a
>> > reasonable error message.
>> >
>> > Also a wonderful feature would be the ability to dynamically hook
>> >
>> > mono_profiler_install_enter_leave (pe_method_enter, pe_method_leave);
>> >
>> > As it is quite expensive. I imagine though this would be non-trivial.
>> >
>> > Greg
>> >
>> > On Tue, Jun 20, 2017 at 9:50 AM, Alex Rønne Petersen <[email protected]>
>> > wrote:
>> >> Hello everyone,
>> >>
>> >> As part of our ongoing effort to make Mono's log profiler useful for
>> >> more scenarios, I'm planning to make it possible to interact with the
>> >> profiler at runtime - you can enable, disable, and tweak specific
>> >> profiler features in certain sections of your application, so you get
>> >> exactly the data that you're interested in. In order to do this, the
>> >> log profiler needs to be able to change its event flags and installed
>> >> callbacks dynamically at runtime.
>> >>
>> >> # The Problem
>> >>
>> >> It is currently impossible for any profiler to reliably change its
>> >> setup at runtime because Mono's profiler API (metadata/profiler.h)
>> >> only allows modifying the most recently installed profiler. Mono
>> >> supports having multiple profilers active at the same time, and we do
>> >> in fact use this feature in the Xamarin platform products. There's no
>> >> way around it: We need to rethink the profiler API. All functions must
>> >> take an explicit MonoProfiler* parameter.
>> >>
>> >> This isn't the only problem with the current API.
>> >>
>> >> Another issue is that multiple callbacks are installed through the
>> >> same function. For example, mono_profiler_install_exception installs
>> >> callbacks for thrown exceptions, exceptional method exits, and
>> >> exception clauses. When I had to add an extra parameter to the
>> >> exception clause callback recently, I introduced
>> >> mono_profiler_install_exception_clause for version 2 of that callback.
>> >> This means that new code will pass NULL to the third parameter of
>> >> mono_profiler_install_exception from now on. This just adds confusion.
>> >> It would be much clearer if the old function had been called
>> >> mono_profiler_install_exception_clause and I'd just been able to
>> >> introduce a mono_profiler_install_exception_v2 function. New users of
>> >> the API will likely wonder why mono_profiler_install_exception_clause
>> >> isn't part of mono_profiler_install_exception since the API has a
>> >> precedent of bundling related callbacks into the same installation
>> >> function.
>> >>
>> >> There are also multiple callbacks in the API that aren't guarded by
>> >> event flags. For example, the code buffer callbacks should logically
>> >> be guarded by MONO_PROFILE_JIT_COMPILATION, but that's a change we
>> >> can't make now as it would be breaking. Another curiosity is that the
>> >> GC handle callbacks are guarded by MONO_PROFILE_GC_ROOTS even though
>> >> it's entirely likely that someone would be interested in GC handles
>> >> but not GC roots (see: Alan McGovern's GC handle profiler). It's also
>> >> odd that the exceptional method exit callback is guarded by
>> >> MONO_PROFILE_EXCEPTIONS when in fact most uses of this callback have
>> >> little to do with profiling exceptions and everything to do with
>> >> keeping track of method entries/exits as with the normal method
>> >> enter/exit callbacks (which are guarded by MONO_PROFILE_ENTER_LEAVE).
>> >>
>> >> We also have callbacks that serve no actual purpose, and never will.
>> >> For example, the notion of a 'class unload' does not exist in the Mono
>> >> runtime. Never has, probably never will. Entire images are unloaded at
>> >> once, so this callback is literally never invoked. I'd actually say
>> >> having that callback there adds negative value to the API. The
>> >> managed/native transition callback was never implemented, either.
>> >>
>> >> Finally, some features in the API have not been maintained or tested
>> >> for years. The call chain sampling API is a great example of this.
>> >> Another example: Did you know that the profiler API supports two
>> >> coverage modes which are mutually exclusive? You might think that
>> >> MONO_PROFILE_COVERAGE is the flag that you're supposed to be using.
>> >> Nope; it's MONO_PROFILE_INS_COVERAGE. The former is implemented in a
>> >> very platform-specific manner that has resulted in it not being
>> >> maintained, tested, or ported fully to new platforms.
>> >>
>> >> In short, the current profiler API is pretty bad. We need a new API.
>> >> Of course, the elephant in the room is backwards compatibility. The
>> >> question is: Do we introduce a new profiler API and make the old one
>> >> 'simply' call the new one? Or do we just replace the old API entirely,
>> >> backwards compatibility be damned?
>> >>
>> >> # The New Profiler API
>> >>
>> >> The new API would not be all that different from the old one. The main
>> >> changes would be:
>> >>
>> >> 1. All functions in the API take an explicit MonoProfiler* parameter.
>> >> 2. Callbacks can be changed safely at runtime.
>> >> 3. One installation function installs exactly one callback.
>> >> 4. You will no longer need to specify event flags.
>> >> 5. Unmaintained and unfinished features (see above) will be removed.
>> >>
>> >> As an example, old code might look like this:
>> >>
>> >> void
>> >> mono_profiler_startup (const char *args)
>> >> {
>> >>     MonoProfiler *prof = malloc (...);
>> >>     profiler_specific_setup (prof);
>> >>     mono_profiler_install (prof, my_shutdown_callback);
>> >>     mono_profiler_install_enter_leave (my_enter_callback,
>> >> my_leave_callback);
>> >>     mono_profiler_set_events (MONO_PROFILE_ENTER_LEAVE);
>> >> }
>> >>
>> >> New code would look like this:
>> >>
>> >> void
>> >> mono_profiler_startup (const char *args)
>> >> {
>> >>     MonoProfiler *prof = malloc (...);
>> >>     profiler_specific_setup (prof);
>> >>     mono_profiler_install (prof);
>> >>     mono_profiler_set_shutdown_callback (prof, my_shutdown_callback);
>> >>     mono_profiler_set_enter_callback (prof, my_enter_callback);
>> >>     mono_profiler_set_leave_callback (prof, my_leave_callback);
>> >> }
>> >>
>> >> We would still use flags internally so we don't slow the runtime down
>> >> with unnecessary profiler API calls, but that will be completely
>> >> hidden from users. All a user would have to worry about is (un)setting
>> >> callbacks, which can be done at any point during an app's lifetime.
>> >>
>> >> Transitioning to the new API should be fairly painless. I'd estimate
>> >> it to take an hour or two at worst for e.g. the log profiler.
>> >>
>> >> # Approach One: Backwards Compatibility
>> >>
>> >> In this approach, we would introduce a new metadata/profiler-v2.h
>> >> header. This header would provide the new API and have no dependencies
>> >> on the old one. The old API would remain in metadata/profiler.h and
>> >> people's code would continue to compile and work. We would need to
>> >> bridge the old API to the new one and make sure that it's done in a
>> >> backwards-compatible way.
>> >>
>> >> The advantage here is fairly obvious: Nobody likes having to rewrite
>> >> their code because the authors of a library decided to change the API,
>> >> especially if that change doesn't carry an obvious benefit to users,
>> >> which it could be argued this change wouldn't for most (all?) current
>> >> users of Mono's profiler API.
>> >>
>> >> On the other hand, this is a significant maintenance burden, both in
>> >> the short and long term. Writing the code to bridge the nonsensical
>> >> aspects of the old API with the new one would be tricky to say the
>> >> least. In addition, there's the risk that any change to the new API in
>> >> the future could break the old API.
>> >>
>> >> # Approach Two: Replacing the API
>> >>
>> >> In this approach, we replace the old API in metadata/profiler.h with
>> >> the new one, with zero regard for backwards compatibility. People's
>> >> code would fail to compile, and old compiled profiler modules would
>> >> fail to run. In both cases, the failures should be fairly loud - a
>> >> compiler error, or a dynamic linker error.
>> >>
>> >> The advantage of this approach is that it's significantly less effort
>> >> to implement and maintain. It also avoids any potential confusion for
>> >> new users of the API, in that there's only one set of functions to
>> >> use.
>> >>
>> >> If we go down this route, all projects that use Mono's profiler API
>> >> would need to change their code slightly, and people would need to
>> >> compile separate versions of their profiler modules if they want to
>> >> support older Mono versions.
>> >>
>> >> # My Opinion
>> >>
>> >> I'm strongly in favor of the second approach. Frankly, as the person
>> >> who'll be implementing and maintaining the new API, I don't
>> >> particularly enjoy the idea of having to also maintain the old one in
>> >> a backwards compatible fashion. I think there are much better things I
>> >> could be working on in Mono's profiling infrastructure.
>> >>
>> >> I also firmly believe that this is the only time we'll have to do such
>> >> a drastic breaking change to the profiler API. This isn't a proposal
>> >> to jump on some fancy new API design fad. Using a mutable global
>> >> variable as an implicit parameter to an entire API was pretty bad
>> >> design, even by 2002 standards. Just by passing an explicit
>> >> MonoProfiler* argument to all API functions, we open ourselves up to
>> >> much easier, backwards-compatible expansion of the API in the future.
>> >>
>> >> Finally, as I mentioned earlier, transitioning to the new API would be
>> >> very easy, and users would have to do it sooner or later anyway, as we
>> >> wouldn't want to keep the old API around forever, even in the first
>> >> approach. Also, in the grand scheme of things, this probably won't
>> >> affect that many people, unlike breaking changes to the core embedding
>> >> API.
>> >>
>> >> What's everyone's thoughts on this?
>> >>
>> >> Regards,
>> >> Alex
>> >> _______________________________________________
>> >> Mono-devel-list mailing list
>> >> [email protected]
>> >> http://lists.dot.net/mailman/listinfo/mono-devel-list
>> >
>> >
>> >
>> > --
>> > Studying for the Turing test
>> _______________________________________________
>> Mono-devel-list mailing list
>> [email protected]
>> http://lists.dot.net/mailman/listinfo/mono-devel-list
>
>



-- 
Studying for the Turing test
_______________________________________________
Mono-devel-list mailing list
[email protected]
http://lists.dot.net/mailman/listinfo/mono-devel-list