The default bug view has changed. See this FAQ.

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Trunk
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Currently the trace JIT is only turned on in chrome code, where the suspicion is that it does not have much effect on perf (the same goes for the method JIT, for that matter).  To unblock other stuff (object shrinking, bug 685358 in part) the tracer needs to be removed, but rather than do that outright it would be good to disable it first and see if tests or browsing are affected.
Blocks: 684505
Created attachment 566361 [details] [diff] [review]
patch
Attachment #566361 - Flags: review?
Attachment #566361 - Flags: review? → review?(dmandelin)
Comment on attachment 566361 [details] [diff] [review]
patch

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

Could you instead do this by modifying the default preference in all.js? That will help with testing by allowing testers to easily turn it back on.
Attachment #566361 - Flags: review?(dmandelin)
The full list of files with tracejit prefs is:

  modules/libpref/src/init/all.js
  js/src/tests/user.js
  toolkit/content/tests/fennec-tile-testapp/defaults/preferences/prefs.js
Created attachment 566364 [details] [diff] [review]
revised
Attachment #566361 - Attachment is obsolete: true
Attachment #566364 - Flags: review?
Attachment #566364 - Flags: review? → review?(dmandelin)
Attachment #566364 - Flags: review?(dmandelin) → review+
Assignee: general → bhackett1024
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda97b0f3e05
Blocks: 690608
https://hg.mozilla.org/mozilla-central/rev/dda97b0f3e05
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
As noted in bug 690608 comment 14 and 15, jstracer still appears to be used.  (We're still hitting an assertion-failure in jstracer.cpp, just running mochitests & xpcshell tests)

Does this mean this bug's tracer-disabling didn't actually take effect like it was supposed to?
Hmm, can mochitests and xpcshell tests change configuration options?  The comment 1 patch disables the tracer, but the comment 4 patch which actually landed just turns it off by default.  I was somewhat worried this would happen, and if the tests are not explicitly modifying the configuration I'd rather do the bulletproof thing and land the comment 1 patch.
build/automation.py.in explicitly sets:

  line 421 -- user_pref("javascript.options.tracejit.content", true);

I wonder why it's setting it and other jit prefs at all...
It looks like it was added to force using the JIT on the TM branch during TM development, and nobody has questioned why it is there since ...

I agree that it's mildly insane.
If I set the pref to "false" in the automation.py.in line from comment 9, that prevents the assertion from firing during mochitests (the only place I've reproduced this), so that seems to be what was missing in my case.

It sounds like we should remove that line.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment on attachment 566361 [details] [diff] [review]
patch

Urgh.  OK, I don't think the patch as landed actually did much vis a vis our testing infrastructure.  automation.py definitely needs to get fixed to not set JIT prefs, but I'd also like to land the patch to completely disable the tracer, to avoid the risk of some other piece of testing infrastructure turning it back on.  If someone needs to compare behavior with/without the tracer, they can just use a build which doesn't do this disabling (e.g. any nightly up to the point when this lands).
Attachment #566361 - Flags: review?(dmandelin)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #566361 - Attachment is obsolete: false
(given that the pref will now be ignored, I imagine we'll probably also want a followup bug to remove all usages of the pref from the codebase (the lines in comment 3 & comment 9), to avoid confusion?)
Created attachment 567596 [details] [diff] [review]
remove JIT prefs from automation.py

Remove JIT prefs from automation.py
Attachment #567596 - Flags: review?(dmandelin)
(In reply to Daniel Holbert [:dholbert] from comment #13)
> (given that the pref will now be ignored, I imagine we'll probably also want
> a followup bug to remove all usages of the pref from the codebase (the lines
> in comment 3 & comment 9), to avoid confusion?)

Right now the idea is to see what happens if the tracer is turned off, while making a minimum of fuss in case it needs to get turned back on in some places.  When the tracer is removed entirely from the code base, the tracejit preferences will definitely need to be removed too.
Comment on attachment 567596 [details] [diff] [review]
remove JIT prefs from automation.py

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

I remembered that this might be in there, but I forgot that it came from a ".py.in" file--I just looked for .py in my grep, so I didn't find anything. Is this patch all you need or will you actually need the jscntxt.cpp patch too?
Attachment #567596 - Flags: review?(dmandelin) → review+
> I remembered that this might be in there, but I forgot that it came from a
> ".py.in" file--I just looked for .py in my grep, so I didn't find anything.
> Is this patch all you need or will you actually need the jscntxt.cpp patch
> too?

Per comment 12, I would like to completely disable the tracer to reduce the risk of some other piece of automation turning the tracer back on and muddying the effect of this patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb5d09b48f03
https://hg.mozilla.org/mozilla-central/rev/cb5d09b48f03

please check tree-management, one of these recent patches regressed V8
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Blocks: 697666
Blocks: 698201
Attachment #566361 - Flags: review?(dmandelin)

Updated

4 years ago
Depends on: 811460
You need to log in before you can comment on or make changes to this bug.