Allow forcing traces

[abgruszecki]

No description provided.

[abgruszecki]

test performance please

[dottybot]

performance test scheduled: 1 job(s) in queue, 0 running.

[dottybot]

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5738/ to see the changes.

Benchmarks is based on merging with master (b4a26ca)

[biboudis]

@AleksanderBG this is CI only right?

[abgruszecki]

@biboudis I actually wanted this to be a proper PR, but then pickling broke. It's WIP for now.

[abgruszecki]

test performance please

[dottybot]

performance test scheduled: 2 job(s) in queue, 1 running.

[abgruszecki]

I checked the stacktrace created by throwing an exception from the body of trace. It appears that neither master trace, nor trace/trace.force from this PR erase completely.

java.lang.RuntimeException                                         
         at dotty.tools.dotc.core.TypeComparer.$anonfun$recur$2(TypeComparer.scala:989)
         at scala.runtime.java8.JFunction0$mcZ$sp.apply(JFunction0$mcZ$sp.java:23)
         at dotty.tools.dotc.reporting.trace$.apply(trace.scala:40)
         at dotty.tools.dotc.reporting.trace$.apply(trace.scala:44)
         at dotty.tools.dotc.core.TypeComparer.recur(TypeComparer.scala:178)

[dottybot]

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5738/ to see the changes.

Benchmarks is based on merging with master (4c4d5a1)

[abgruszecki]

@OlivierBlanvillain this is the trace.force syntax that forces a specific trace without requiring full project recompilation, demanding somewhat arcane commandline options or printing out more than one wants. It has been indispensable to understand what isSubType/constrainPatternType is doing. It inlines just as well as previous trace. It has no observable impact on performance. Can we ship it?

[allanrenucci]

Use a field TraceSyntax(isForced: Boolean)? I would expect it is cheaper

[abgruszecki]

Could you be more clear on what it is you call a field? The goal here is to override isForced with a final val, which should be guaranteed to be inlined whenever referenced, which would then mean that we get rid of useless branches in the inlined logging methods. This is somewhat moot, since forceInline does not seem to inline nested methods properly ( see #5738 (comment) ), but I don't see how making isForced a constructor argument makes it any cheaper.

[allanrenucci]

Here the final val implements an abstract one. It will be compiled to a virtual method call, I don't think the compiler can inline in this case. If isForced is a constructor argument, it will be compiled to a field. A field access is much cheaper than a virtual method call.

[abgruszecki]

I see the reasoning, but all the methods that access isForced are @forceInline, so after the body is inlined the compiler should see that the member being accessed is a final val and therefore is inlineable, no? Anyway, this PR has no impact on performance and after we bootstrap we will probably rewrite all the methods in this class to inline, which will guarantee that they are inlined the way we want.

Side note: I am bookmarking this discussion to show the next person that tries to convince me it's a good idea to rely on the optimizer for inlining.

[OlivierBlanvillain]

I'm not very familiar with the tracing logic so I might not be best person to review this code, but LGTM

[OlivierBlanvillain]

Are you using this?

[abgruszecki]

This replaces the previous trace object - now wherever there are calls like trace("isSubtype") { ... } it's possible to replace trace with trace.force to have only that particular trace printed.

[OlivierBlanvillain]

I see. Then that might be a could place to put two lines of scaladoc...

[odersky]

What is the point of this? Is isForced just another way to enable tracing besides Config.tracingEnabled?

My gut reaction is this is a bad idea. All control of reporting behavior should be in Config or config.Printers. Don't spread it out to other switches.

[abgruszecki]

trace.force is an easy way to force a specific trace to be printed regardless of any other setting. Currently there's no good way to enable just a trace without enabling its corresponding printer - that requires changing the printer to default, enabling Config.tracingEnabled, enabling Ylog, and recompiling the entire codebase. With this PR, all one needs to do is just add .force.

I personally found the .force way of doing things much easier even when needing to cherry-pick and undo the changes constantly. Having this actually in the codebase would be helpful, and seeing as using it is completely voluntary and constrained to one's workflow, I cannot really see how it could hurt.