Add IReactorListener interface

[saul]

This PR adds an interface to listen to FCS reactor thread events. This allows users of FCS to override how reactor events are logged.

The methods that are only used from FCS tests have not been added to the IReactorListener interface.

[cartermp]

Since this is new code, can we consider using the single-underscore self identifier here?

[KevinRansom]

@cartermp, I think fcs may use a bootstrap compiler thats pre- _.SomeMethod. I sort of half remember some issue due to fcs.

[KevinRansom]

Can we see, if we can use a single _. If that is not possible, then I am fine with this change as is.

Thanks for this

[TIHan]

What are the use cases for listening to reactor events besides just giving FCS users the ability to listen to it publicly?

The reactor thread is an implementation detail and should absolutely be hidden away. The reason it exists is because the compiler has not proven it can can do parallel work safely (yet). It is a mechanism to make the compilation work happen in a single-threaded fashion.

Exposing it publicly like this really opens a door which FCS users will rely on the behavior of the background thread which is something to avoid.

[saul]

Hi Will - thanks for taking a look at this.

The use case is showing to the user when the reactor thread has been busy for some period of time. See the GIF in this PR for example: JetBrains/resharper-fsharp#127 It's currently implemented by regex matching strings with a TraceListener, which isn't ideal.

The reactor thread is an implementation detail like you said, but it is front and centre in most performance-related issues. Knowing what the reactor thread is doing in a period of slowness is pivotal (e.g. when you've been waiting 45 seconds for semantic colourisation). On the first day of the feature being added to Rider we noticed an issue that was causing more typechecks than expected.

I can understand your wariness of exposing something that you're wanting to get rid of, but this PR doesn't expose anything that you can't already elicit from a TraceListener. It's just that regex matching trace strings is far slower than using an IReactorListener.

[KevinRansom]

@saul , do you have any suggestions how can expose this in "less direct" manner. @TIHan has a very valid point about not wanting to live with the reactor forever, however, we do have the reactor now and the problem you are solving is a real issue. This PR adds public APIs to FSharp.Compiler.Private, and so we have to care about them for a long time.

[saul]

Hi Kevin - unfortunately I can't really see another way of getting the information we need without an interface like this. I am obviously very open to suggestions too - I understand extending the public API is always a hard sell.

I did notice that the reactor thread is already exposed (although not useful in this case) on FSharpChecker as CurrentQueueLength. The doc comment explains that it's for debugging - if I added a similar comment to the doc comments of the interface I've added, would that allay your concerns?

[KevinRansom]

@saul , I had the same feeling, when I looked at it too. Let's give it a couple of days stewing, something may occur to one of us. I still think it is necessary, I am just hoping there is someway we can expose it less directly.

[TIHan]

@saul, that makes sense. So this is mainly for performance tracing?

What do you think about adding new ETW events instead of having to expose a listener? We already use ETW events in FCS for tracing performance. See Logger.fs/fsi and their uses; they are also not public.

[KevinRansom]

@TIHan
I think the motivation for this is to add some UI to jetbrains to display reactor activity as it is happening, not by reviewing a dump after the program has finished execution. (link here: JetBrains/resharper-fsharp#127). I think they can achieve that using ETW and writing an ETW listener, but I am not sure if they would get the same performance in an IDE. You have experience of doing ETW tracing, do you think they could achieve good performance that way?

[TIHan]

I'm not sure I am in favor of allowing reactor behavior be exposed directly for activity info display in an editor experience of any kind.

However, I think it's fair to have some API give info on whether or not something is running/busy. At the top of my head, I don't know what the API looks like. Perhaps, a much lesser version of the current API without knowing that it is from the reactor thread.

[saul]

@KevinRansom the downside of using ETW is that it's Windows only, whereas this implementation is cross platform.

@TIHan I think the change should be judged on its usefulness as opposed to whether you agree with how it's planned to be used. I just want to stress that all of the reactor information that you're worried about being exposed is already exposed via trace messages anyway. Indeed the feature that we want to implement off the back of this PR has already been implemented, but it's implemented in a kludgey way by regex matching the trace messages.

I don't fully understand your concerns about exposing information about the reactor thread. Knowing that it exists and that it processes work sequentially is critical to understanding how/why FCS is behaving in a certain way. It's critical for debugging, which is why @dsyme added the trace messages way back when: #3061

[TIHan]

the downside of using ETW is that it's Windows only, whereas this implementation is cross platform.

ETW is windows only, but the actual API we use for logging, EventSource, should be cross-platform. https://docs.microsoft.com/en-us/dotnet/core/diagnostics/logging-tracing

EventSource is the primary root .NET Core tracing API.
Available in all .NET Standard versions.
Only allows tracing serializable objects.
Writes to the attached event listeners.
.NET Core provides listeners for:
   .NET Core's EventPipe on all platforms
   Event Tracing for Windows (ETW)
   LTTng tracing framework for Linux

I think the change should be judged on its usefulness as opposed to whether you agree with how it's planned to be used.

I disagree. The change must be judged on how it's going to be used because of potential abuse. Developers building tools based on the behavior of the internal reactor thread is worrisome and can have consequences.

reactor information that you're worried about being exposed is already exposed via trace messages anyway

This is not the same as an API exposing reactor information and events. Trace messages were meant for logging while an API can be used for anything.

It's critical for debugging, which is why @dsyme added the trace messages way back when

Sure, but looking at what they were trying to accomplish could be improved by adding more events to our logger that use EventSource underneath. At the time of that PR, we didn't have our logger.

--

I'm not saying we can't expose events that can get triggered on the reactor thread; I'm saying we must be careful what we expose as to limit abuse. Yes, we do have APIs on FSharpChecker that can get information from the background compiler which uses the reactor, but it's pretty limited. It's reasonable to start exposing more events; we just need to do it in targeted way that is based on how it will be used. If the use case is determining when the background compiler is busy, then we could add two events, as an example, on FSharpChecker: val BackgroundWorkStarted: IEvent<unit>, val BackgroundWorkStopped: IEvent<unit>.

[dsyme]

I do think ETW/EventSource events are designed for this kind of observation of operational properties of systems and we should just embrace that. If I'd done that systematically in the codebase from earlier on we'd have much better observationality today.

However, I think it's fair to have some API give info on whether or not something is running/busy.

@saul, @auduchinok could you outline what the minimum you need is to support the IDE features? Just a busy/not-busy signal? Number of remaining work items in the reactor queue?

[KevinRansom]

@saul, can we turn this into an issue that we need a mechanism to get visibility of some aspects of the properties. It looks like the public interface approach is not going to be an acceptable solution and as such, this PR is probably not the best place to have that conversion.

Thanks

Kevin

The consensus is that public API's are not the way to expose this information.

[cartermp]

Will close this out since the consensus seems to be that we don't want this.