989152 - Tracelogger: Log the ion compilation passes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Make it possible to log the ion compilation passes. To know which part of the compiler takes the most time etc.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: tl2-log-flags

I don't want to log the ion compilation every time. So I added a way to filter textIds using TLLOGS=IonCompiler or even specific textIds like TLLOGS=GC,GCAllocating,GCSweeping. This is the beginning of fixing bug 944698, which would allow TLFILTER with a specific script.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: tl2-log-compilation

This added the actual logging of the different passes. Should be straightforward, but I went for Brian to review this, to know if it would be ok to add a way to get mainThread in CompileWrapper.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8398244 [details] [diff] [review]
tl2-log-flags

Review of attachment 8398244 [details] [diff] [review]:
-----------------------------------------------------------------

very, very nice!

::: js/src/TraceLogging.cpp
@@ +71,5 @@
>      "TraceLogger failed to process text",
>      "Bailout",
>      "Baseline",
>      "GC",
>      "GCAllocating",

Because I just see it: "GCAllocation"

@@ +75,5 @@
>      "GCAllocating",
>      "GCSweeping",
>      "Interpreter",
>      "Invalidation",
> +    "IonCompiling",

"IonCompilation"

@@ +80,1 @@
>      "IonLink",

"IonLinking"

@@ +148,5 @@
>  
>      StackEntry &stackEntry = stack.pushUninitialized();
>      stackEntry.treeId = 0;
>      stackEntry.lastChildId = 0;
> +    stackEntry.logged = true;

How about changing this to "ignored" (and toggle true and false, of course)?

@@ +447,4 @@
>          startEvent(TraceLogger::TL, start);
>          stopEvent();
>      }
>      

pre-existing nit: remove whitespace

@@ +452,5 @@
>      startEvent(id, start);
>  }
>  
> +TraceLogger::StackEntry &
> +TraceLogger::getParent()

This name doesn't give any hint that this is about getting the next-innermost non-ignored entry. Maybe getNonIgnoredParent? (No, I don't like it, but I can't think of anything better.)

@@ +464,5 @@
>  void
>  TraceLogger::startEvent(uint32_t id, uint64_t timestamp)
>  {
> +    // When a textId is disabled, a stack entry still needs to get pushed,
> +    // together with an annotation that nothing needs to get done when logging

s/get/be/, and s/logging/receiving/, as logging is exactly what *doesn't* happen

@@ +626,5 @@
> +        /*NOTREACHED*/
> +    }
> +
> +    enabledTextIds[TraceLogger::TL_Error] = true;
> +    for (uint32_t i = 1; i < TraceLogger::LAST; i++) {

nit: no braces needed

::: js/src/TraceLogging.h
@@ +309,5 @@
>      // Helper functions to get/save a tree from file.
>      void getTreeEntry(uint32_t treeId, TreeEntry *entry);
>      void saveTreeEntry(uint32_t treeId, TreeEntry *entry);
>  
> +    // Return the first StackEntry that has been logged.

See comment in the .cpp file

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 8398250 [details] [diff] [review]
tl2-log-compilation

Review of attachment 8398250 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CompileWrappers.cpp
@@ +31,5 @@
> +CompileRuntime::mainThread()
> +{
> +    JS_ASSERT(onMainThread());
> +    return &runtime()->mainThread;
> +}

An alternative to this method is TlsPerThreadData.get()

Comment 5

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71945517001a
https://hg.mozilla.org/integration/mozilla-inbound/rev/789e7f8b3603

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/71945517001a
https://hg.mozilla.org/mozilla-central/rev/789e7f8b3603