Closed Bug 1071546 Opened 5 years ago Closed 5 years ago

Build tracelogger by default

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: h4writer, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Currently tracelogger needs a configure flag before you can use it. This is because as is tracelogger incurs a really small performance penalty, even when disabled.

Tracelogger has already decreased its footprint considerable and not too much work should be needed before enabling it fully. We need to conditionally call the jit instrumentation when we are not logging and enable it when needed.
Blocks: 1065722
+1 for fewer build configurations.
Attached patch TL-enable-by-default (obsolete) — Splinter Review
This would enable tracelogger by default, with a configure option to disable it. We can later decide to remove that option.
Assignee: nobody → hv1989
I see no regressions on octane anymore. I'll doublecheck everything before committing though ;). This let us build tracelogger by default, though everything is disabled by default too.

So you'll need to specify TLLOG=Default TLOPTIONS=EnableMainThread before tracelogger is reporting something. I'll update the documentation after this landed. Having this enabled by default will open new possibilities. (Currently looking to log things for the devtools, but more will be possible :D)
Attachment #8494493 - Attachment is obsolete: true
Attachment #8494558 - Flags: review?(till)
Comment on attachment 8494558 [details] [diff] [review]
TL-enable-by-default

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

\o/

r=me with pre-existing nits addressed.

I forgot the exact rules, but please make sure you don't need a build peer's review for the configure.in changes. Or request one, of course.

::: js/src/vm/TraceLogging.cpp
@@ +854,5 @@
>              printf(
>                  "\n"
>                  "usage: TLOPTIONS=option,option,option,... where options can be:\n"
>                  "\n"
> +                "  EnableMainThread        Start logging the mainThread immediately.\n"

s/the mainThread/the main thread/

@@ +855,5 @@
>                  "\n"
>                  "usage: TLOPTIONS=option,option,option,... where options can be:\n"
>                  "\n"
> +                "  EnableMainThread        Start logging the mainThread immediately.\n"
> +                "  EnableOffThread         Start logging the off mainThread immediately.\n"

This message is somewhat confusing. How about "Start logging helper threads immediately"?
Attachment #8494558 - Flags: review?(till) → review+
Comment on attachment 8494558 [details] [diff] [review]
TL-enable-by-default

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

(In reply to Till Schneidereit [:till] from comment #4)
> I forgot the exact rules, but please make sure you don't need a build peer's
> review for the configure.in changes. Or request one, of course.

I couldn't really find the documentation on this. So I'm requesting r+ from glandium to be sure.
Attachment #8494558 - Flags: review?(mh+mozilla)
Attachment #8494558 - Flags: review?(mh+mozilla) → review+
Could this be the cause of a 40% regression on 32-bit no-asm asm-skinning on awfy? Odd, it's just that one test. But nothing else in the commit range looks relevant.
https://hg.mozilla.org/mozilla-central/rev/44c68f942d20
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Hi, sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-central&revision=b9fd2074d588 since with the landings of this changesets we had permafailures in ggc tests like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=661496&repo=mozilla-central
Status: RESOLVED → REOPENED
Flags: needinfo?(hv1989)
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/fbf7bd61c853
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla36 → mozilla37
Tracelogger commits appear to have regressed kraken by 6.4% on 32-bit. (Oddly, no significant change on 64-bit.) The change appears to be entirely in kraken-fft.
(In reply to Alon Zakai (:azakai) from comment #11)
> Tracelogger commits appear to have regressed kraken by 6.4% on 32-bit.
> (Oddly, no significant change on 64-bit.) The change appears to be entirely
> in kraken-fft.

I opened bug 1116524 for this
Flags: needinfo?(hv1989)
This also introduced a whole bunch of static initializers:



Regression: Mozilla-Inbound - Number of Constructors - CentOS release 5 (Final) - 2.41% increase
------------------------------------------------------------------------------------------------
    Previous: avg 83.000 stddev 0.000 of 12 runs up to revision e2d5ec4286aa
    New     : avg 85.000 stddev 0.000 of 12 runs since revision ef7a85ec6595
    Change  : +2.000 (2.41% / z=0.000)
    Graph   : http://mzl.la/1EGMdVT

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e2d5ec4286aa&tochange=ef7a85ec6595

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/fbf7bd61c853
    : Hannes Verschore <hv1989@gmail.com> - Bug 1071546 - TraceLogger: Build tracelogger by default, r=till,glandium
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1071546

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/3ccf3fac28b7
    : Hannes Verschore <hv1989@gmail.com> - Bug 996509 - TraceLogger: Make it possible to toggle TraceLogger_Script on and off, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=996509

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/648947a5a445
    : Hannes Verschore <hv1989@gmail.com> - Bug 996509 - TraceLogger: Make it possible to toggle TraceLogger_Engine on and off, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=996509

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/16aa61577e95
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072903 - TraceLogger: Part 1: Remove static array and use function to retrieve the tracelogger text descriptions, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072903

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/37770c7a3f0f
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072903 - TraceLogger: Part 1.5: Fix iterations of stack when getting enabled, r=nbp
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072903

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/63b734ac95b2
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072903 - TraceLogger: Part 2: Adjust how to retrieve the tracelogger text ids, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072903

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/12348c397b08
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072903 - TraceLogger: Part 3: Refactor into TraceLoggerGraph, TraceLoggerThread and TraceLoggerThreadState, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072903

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/04988ab0c1da
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072903 - TraceLogger: Part 4: Free things accordingly, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072903

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/aad64d6ad822
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072903 - TraceLogger: Part 5: Log whenever tracelogger gets enabled or disabled, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072903

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/20dc3d4a9303
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072903 - TraceLogger: Part 6: Add locking logic for TraceLoggerGraph, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072903

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/2d0b50d68320
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072903 - TraceLogger: Part 7: Disable TraceLoggerGraph by default, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072903

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/aa41463b912e
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072906 - TraceLogger: Part 1: Make it possible to toggle text ids dynamically in Baseline and IonMonkey, r=jandem
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072906

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/dd75dcb78dc7
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072906 - TraceLogger: Part 2: Improve the interface to setup the tracelogger, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072906

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/bad0e92bd026
    : Hannes Verschore <hv1989@gmail.com> - Bug 1072910 - TraceLogger: Create hooks for the debugger, r=bbouvier
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1072910

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/092b29ea64e0
    : Hannes Verschore <hv1989@gmail.com> - Bug 1083694 - TraceLogger: Part 2: Split meaning of Script into AnnotateScript and Script (called), r=till
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1083694

  * and 8 more

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=1072903 - Tracelogger: Split the graph creation from logging
  * http://bugzilla.mozilla.org/show_bug.cgi?id=1072906 - Tracelogger: Add way to enable/disable logging in IM/Baseline
  * http://bugzilla.mozilla.org/show_bug.cgi?id=1083694 - TraceLogger: Cleanups.
  * http://bugzilla.mozilla.org/show_bug.cgi?id=1072910 - Tracelogger: Create the hooks for Debugger
  * http://bugzilla.mozilla.org/show_bug.cgi?id=1071546 - Build tracelogger by default
  * http://bugzilla.mozilla.org/show_bug.cgi?id=996509 - Tracelogger: Make it possible to toggle when scripts get logged.
_______________________________________________
dev-tree-management mailing list
dev-tree-management@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-tree-management
You need to log in before you can comment on or make changes to this bug.