Closed Bug 989152 Opened 6 years ago Closed 6 years ago

Tracelogger: Log the ion compilation passes

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files)

Make it possible to log the ion compilation passes. To know which part of the compiler takes the most time etc.
Attached patch tl2-log-flagsSplinter Review
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.
Assignee: nobody → hv1989
Attachment #8398244 - Flags: review?(till)
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.
Attachment #8398250 - Flags: review?(bhackett1024)
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
Attachment #8398244 - Flags: review?(till) → review+
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()
Attachment #8398250 - Flags: review?(bhackett1024) → review+
Depends on: 944392
https://hg.mozilla.org/mozilla-central/rev/71945517001a
https://hg.mozilla.org/mozilla-central/rev/789e7f8b3603
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 995490
You need to log in before you can comment on or make changes to this bug.