(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 <gregoryyou...@gmail.com> 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 <a...@alexrp.com> 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
>> Mono-devel-list@lists.dot.net
>> http://lists.dot.net/mailman/listinfo/mono-devel-list
>
>
>
> --
> Studying for the Turing test
_______________________________________________
Mono-devel-list mailing list
Mono-devel-list@lists.dot.net
http://lists.dot.net/mailman/listinfo/mono-devel-list
