search

haskell / core-libraries-committee

96 stars 16 forks source link

Make Debug.Trace.{traceEventIO,traceMarkerIO} faster #291

Open hsyl20 opened 1 week ago

hsyl20 commented 1 week ago

Proposal: make base:Debug.Trace.{traceEventIO,traceMarkerIO} faster when tracing is disabled by checking if the RTS flag enabling user event tracing is enabled before doing any work with the lazy string argument.

mixphix commented 1 week ago

How do you expect this to play out regarding the RTS flag removal from base #289?

Certainly this is a user-facing change to comsumers of base, but the implementation of base:Debug.Trace now lives inside ghc-internal:GHC.Internal.Debug.Trace. So there is not much that needs to be done by the CLC, am I wrong @Bodigrim?

Bodigrim commented 1 week ago

Certainly this is a user-facing change to comsumers of base, but the implementation of base:Debug.Trace now lives inside ghc-internal:GHC.Internal.Debug.Trace. So there is not much that needs to be done by the CLC, am I wrong @Bodigrim?

Pretty much everything lives inside ghc-internal now. The agreement is that whatever is re-exported from base falls under CLC purview, even if the implementation is inside ghc-internal / ghc-prim / ghc-bignum.

mixphix commented 1 week ago

Right. Seeing as there's a ticket (opened before this one) to remove the flags from base, but this ticket wants to use them in base to improve the performance of two functions, I'm kind of at a loss for what the general plan of the GHC team is on whether the flags are stable or not. Are these the only two functions in Debug.Trace that can be optimized this way?

maralorn commented 1 week ago

Well, base is one of the few packages for which it is totally fine to depend on ghc-internal. Changing RTSFlags wouldn't cause an API changing modification of the new implementation of the functions discussed here, would it?

doyougnu commented 1 week ago

but this ticket wants to use them in base to improve the performance of two functions

There is some miscommunication. This proposal was opened because of the runtime performance change. The implementation relies on a new module in ghc-internal called GHC.RTS.Flags.Test, its sole purpose is a top level constant that detects if the RTS has eventlog support or not, but crucially this is not added to base. The new module is exposed in ghc-internal but is only used by the Debug.Trace implementation which is still re-exported to base. The goal of this proposal is approval for the MR because of the runtime performance changes.

How does this proposal relate to #289

It doesn't, the goal is not to move Debug.Trace or add flags to base, only alter Debug.Trace.traceEvent and Debug.Trace.traceEventIO's implementation. Both are re-exported from ghc-internal to base just as before. The newer implementation does not rely on either GHC.RTS.Flags or GHC.Stat which is what #289 is concerned with.

Does that make things more clear?

Ericson2314 commented 1 week ago

Excellent separation of concerns! The CLC is asked to sign off on a change in the performance contract (external interface), but the mechanism that implements that remains private because it relies on unstable interfaces.

hsyl20 commented 1 day ago

Well, base is one of the few packages for which it is totally fine to depend on ghc-internal. Changing RTSFlags wouldn't cause an API changing modification of the new implementation of the functions discussed here, would it?

Yes whatever we decide for RTSFlags shouldn't impact this.

May I ask for a vote on this?