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.
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.
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
Attachment #566364 - Flags: review? → review?(dmandelin)
Attachment #566364 - Flags: review?(dmandelin) → review+
Status: NEW → RESOLVED
Closed: 8 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.
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #566361 - Attachment is obsolete: false
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/mozilla-central/rev/cb5d09b48f03 please check tree-management, one of these recent patches regressed V8
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.