search

JunoLab / Traceur.jl

Other
318 stars 15 forks source link

Merge static tracing from Analyzer #7

Closed jamii closed 6 years ago

jamii commented 6 years ago

This is a bit tricky because all the reflection functions take (function, argtypes) but the static tracing can't get actual functions, only function types. So I have lots of sketchy calls into Julia internals to reproduce what code_typed etc do. I added StaticCall, DynamicCall <: Call to keep this muck separate from the existing Traceur code as much as possible.

I'm not sure what to do about unifying the interface. The options and defaults for each are going to be different so passing that stuff through will be a pain. Left to myself I would get rid off the callbacks and do something like:

function trace(f; tracer=DynamicTracer())
  call_graph = call_graph(f, tracer)
  warnings = warnings(call_graph)
  print_warnings(warnings)
end

call_graph(f, tracer::DynamicTracer) = ...
call_graph(f, tracer::StaticTracer) = ...

warnings(call_graph) = [(call, analyze(call)) for call in call_graph)]
MikeInnes commented 6 years ago

LGTM, thanks for the patch (and apologies for the late response).

I think @pfitzseb is best to comment on API changes given that he'll be integrating this into Juno, but I think the streaming design is worth holding on to. It's probably not an issue for static traces but dynamic ones can potentially loop or take a while; you want to (a) see that it's doing something and (b) possibly ctrl-c in the middle of a run, but still get whatever output you can.

The other thing the current approach tries to get right is having things in the "right order" – i.e some warnings should come up in pre-order and some in post, depending on what's generally a cause or an effect of instability later down the line. You could still do that with call_graph but it'd be a bit more complex.

jamii commented 6 years ago

and apologies for the late response

No worries, I don't have much time to work on this at the moment either. But the good news is my job description now officially includes "Julia tooling and support" so there definitely will be time eventually.

dynamic ones can potentially loop or take a while

I hadn't thought about that at all. Streaming makes sense now.

MikeInnes commented 6 years ago

That's awesome -- looking forward to seeing what comes out of that!