This bug takes care of Tracelogger integration in Odin, for more precise profiling.

TraceLogger, when creating a text identifier for a given function (to put in the scripts list), needs a filename at least. The text id is created with the CompileOptions. In case of dynamic linking failure, we don't provide a filename in the CompileOptions, text id creation crashes because it has an assertion that uses the filename. This one-liner fixes it. Note that we don't provide lineno and colno either. More work could be made to retrieve it from the ScriptSource, I suppose, but that's an edge case so we shouldn't be too interested in this one.

With this, tracelogger is now capable of knowing when time is spent in: - OdinMonkey compilation (redundant with the compilation time report) - OdinMonkey, aka running under Odin.

This one adds support for logging: - Odin linking. - Odin entry trampoline calls (OdinEntry). Logging from the time we enter CallAsmJS, as splitting it into two parts (C++ CallAsmJS and later the assembly trampoline) didn't add more value in my opinion. - Odin exit trampoline calls (into the interpreter and Ion). These ones are less important, unless for very specific test cases. Thinking about it, I think we'd want them but maybe not enable them by default. Any thoughts?

Comment on attachment 8418154 [details] [diff] [review] Main integration Review of attachment 8418154 [details] [diff] [review]: ----------------------------------------------------------------- So every engine currently reports: LogStartScript LogStartEngine (a.k.a. interpreter/baseline/ionmonkey) LogStopEngine LogStopScript This is because the graphs groups the information per script. It should be better if we can log a filename/lineno so this get's grouped there... Is that possible? Else the patch looks good!

Comment on attachment 8418156 [details] [diff] [review] Extra loggings Review of attachment 8418156 [details] [diff] [review]: ----------------------------------------------------------------- OdinLinking looks interesting for me. It might also be good to again have a TraceLoggerStartScript before that. So it get's grouped under the right filename:lineno:co I'm don't really understanding the purpose of - OdinEntry - OdinFFIInterpreterExit - OdinFFIIonExit I think you want them to see how long the trampoline duration is? In that case I would just create a "Trampoline" text id. And start/stop in the right places? It is easily visible about which trampoline is busy, by looking at which engine was running and to which engine we go... I don't think we need to have specific names for these...

Comment on attachment 8418154 [details] [diff] [review] Main integration Review of attachment 8418154 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/AsmJS.cpp @@ +7100,5 @@ > + if (cx->isJSContext()) > + logger = TraceLoggerForMainThread(cx->asJSContext()->runtime()); > + else > + logger = TraceLoggerForCurrentThread(); > + AutoTraceLog log(logger, TraceLogger::OdinCompilation); Since it's the top-level of the whole process, I'd like to keep js::CompileAsmJS pretty tidy; perhaps you could wrap this logic in an AutoTraceLogAsmJS class so that js::CompileAsmJS contains only a single: AutoTraceLogAsmJS log(cx); ?

Comment on attachment 8418156 [details] [diff] [review] Extra loggings Review of attachment 8418156 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/vm/TraceLogging.h @@ +121,5 @@ > _(IonLinking) \ > _(IonMonkey) \ > _(MinorGC) \ > _(OdinCompilation) \ > + _(OdinEntry) \ Oh, I forgot in the previous review too: I've generally avoided using "Odin" as a prefix anywhere in the code; can you use AsmJS as the prefix?

Comment on attachment 8418152 [details] [diff] [review] Fix link failure issue Added this patch in bug 1009603, as it's part of the failures.

Was just looking into some windows performance issue using tracelogger. Sadly it doesn't understand odinmonkey yet. Would it be hard to refactor this to work on tip?

Not too hard (it needs to be entirely rewritten), but not on my radar right now. I can get back to it later, though.

Getting back on it these days. I've got a basic patch working, but I'd like to make it work for both baseline/ion compiled function, at the function level.