Closed
Bug 693815
Opened 13 years ago
Closed 13 years ago
Disable jstracer
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(3 files)
998 bytes,
patch
|
Details | Diff | Splinter Review | |
2.88 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #566361 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #566361 -
Flags: review? → review?(dmandelin)
Comment 2•13 years ago
|
||
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)
Comment 3•13 years ago
|
||
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
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #566361 -
Attachment is obsolete: true
Attachment #566364 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #566364 -
Flags: review? → review?(dmandelin)
Updated•13 years ago
|
Attachment #566364 -
Flags: review?(dmandelin) → review+
Updated•13 years ago
|
Assignee: general → bhackett1024
Assignee | ||
Comment 5•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda97b0f3e05
Comment 6•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dda97b0f3e05
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Comment 7•13 years ago
|
||
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?
Assignee | ||
Comment 8•13 years ago
|
||
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.
Comment 9•13 years ago
|
||
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.
Comment 11•13 years ago
|
||
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.
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Comment 12•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•13 years ago
|
Attachment #566361 -
Attachment is obsolete: false
Comment 13•13 years ago
|
||
(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?)
Assignee | ||
Comment 14•13 years ago
|
||
Remove JIT prefs from automation.py
Attachment #567596 -
Flags: review?(dmandelin)
Assignee | ||
Comment 15•13 years ago
|
||
(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 16•13 years ago
|
||
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+
Assignee | ||
Comment 17•13 years ago
|
||
> 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.
Assignee | ||
Comment 18•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb5d09b48f03
Comment 19•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cb5d09b48f03 please check tree-management, one of these recent patches regressed V8
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Attachment #566361 -
Flags: review?(dmandelin)
You need to log in
before you can comment on or make changes to this bug.
Description
•