Closed Bug 1031068 Opened 6 years ago Closed 5 years ago

Raytrace is slower in shell/browser build on tbpl

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files, 1 obsolete file)

I was looking into performance differences on windows when noticing my own windows shell build gives better scores than the ones compiled on tbpl.

After investigating I stumbled upon the fact it is caused by "--enable-js-diagnostics":

With --enable-js-diagnostics:
octane-raytrace: 36667

Without --enable-js-diagnostics:
octane-raytrace: 61642

I found two bugs about it:
Bug 705020 - remove enable-js-diagnostics flag from nightly mozconfig
Bug 712642 - Remove --enable-js-diagnostics from aurora/beta mozconfigs

From these bugs I would think we should have no --enable-js-diagnostics in our config flags at all. But apparently we still do.
Upon querying with terrence, he said he would like to revive "--enable-js-diagnostics". So I'm leaving this bug in his hands now. Possibly there is something stupid going on when "--enable-js-diagnostics" is enabled.


I tried checking linux, but this linux pc has been doing strange things wrt performance testing, but there also seems a difference.
Flags: needinfo?(terrence)
Blocks: 1009221
All this stuff should be disabled in our release builds. It's only nightly and aurora that gets it.
(In reply to Bill McCloskey (:billm) from comment #1)
> All this stuff should be disabled in our release builds. It's only nightly
> and aurora that gets it.

That is correct. It just skews results on AWFY (windows machine) a bit. (It takes nightly builds). Which is a bit unfortunate, esp. if it isn't really used or doesn't give extra information.

Talked with terrence further and I'm gonna look which part of "JS_CRASH_DIAGNOSTICS" causes this. Most likely we can just disable that part.
Flags: needinfo?(terrence)
>Bug 705020 - remove enable-js-diagnostics flag from nightly mozconfig

That was for Firefox on Android only.

Our desktop mozconfigs still have the option: http://mxr.mozilla.org/mozilla-central/search?string=enable-js-diagnostics&find=nightly
Well, we do get good data from this option. The compartment mismatch checking has found a number of bugs. I'm less sure about the GC poisoning; a lot of it was added pretty recently. However, it's found one bug for me on tryserver (although that might have been a debug build). Given that it's the raytrace benchmark, it's probably the GC poisoning that's slowing us down.

We might be able to avoid some of the overhead from the GC poisoning by making it a runtime pref.
Attached patch Disable JS_POISON in opt builds (obsolete) — Splinter Review
This disables the poisoning in non-debug builds. Was easiest to do.

We could also make it possible to enable/disable it on runtime, like an environment variable?
Assignee: nobody → hv1989
Attachment #8447116 - Flags: review?(terrence)
Comment on attachment 8447116 [details] [diff] [review]
Disable JS_POISON in opt builds

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

Disabling this wholesale means we will not know whether any particular nightly crash was caused by UAF --  a /very/ important piece of information. As discussed on IRC, let's introduce an environment variable to switch off the poisoning at runtime in environments that are trying to benchmark.
Attachment #8447116 - Flags: review?(terrence)
Attachment #8447116 - Attachment is obsolete: true
Attachment #8447934 - Flags: review?(terrence)
Actually not sure if we do "static variables"
Comment on attachment 8447934 [details] [diff] [review]
Make it possible to disable poison at runtime

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

Statics are fine here as the environment applies to all runtimes.

::: js/src/jsutil.h
@@ +279,5 @@
> +{
> +    static bool inited = false;
> +    static bool poison = true;
> +    if (!inited) {
> +        char *env = getenv("DISABLE_CRASH_DIAGNOSTICS");

Let's call this "JSGC_DISABLE_POISONING" instead.
Attachment #8447934 - Flags: review?(terrence) → review+
This diagnostic never caught anything, unfortunately. We can remove this and look elsewhere now.
Attachment #8448113 - Flags: review?(jcoppeard)
Duplicate of this bug: 1009221
Attachment #8448113 - Flags: review?(jcoppeard) → review+
Hannes, how are we doing here now?
Flags: needinfo?(hv1989)
The first patch fixed the performance difference w/wo --enable-js-diagnostics. So this can get closed.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(hv1989)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.