Last Comment Bug 693815 - Disable jstracer
: Disable jstracer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on: 811460
Blocks: 684505 690608 697666 698201
  Show dependency treegraph
 
Reported: 2011-10-11 14:22 PDT by Brian Hackett (:bhackett)
Modified: 2012-11-13 14:29 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (998 bytes, patch)
2011-10-11 14:51 PDT, Brian Hackett (:bhackett)
no flags Details | Diff | Review
revised (2.88 KB, patch)
2011-10-11 14:59 PDT, Brian Hackett (:bhackett)
dmandelin: review+
Details | Diff | Review
remove JIT prefs from automation.py (1.23 KB, patch)
2011-10-17 15:23 PDT, Brian Hackett (:bhackett)
dmandelin: review+
Details | Diff | Review

Description Brian Hackett (:bhackett) 2011-10-11 14:22:35 PDT
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.
Comment 1 Brian Hackett (:bhackett) 2011-10-11 14:51:38 PDT
Created attachment 566361 [details] [diff] [review]
patch
Comment 2 David Mandelin [:dmandelin] 2011-10-11 14:53:10 PDT
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.
Comment 3 David Mandelin [:dmandelin] 2011-10-11 14:55:38 PDT
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
Comment 4 Brian Hackett (:bhackett) 2011-10-11 14:59:02 PDT
Created attachment 566364 [details] [diff] [review]
revised
Comment 5 Brian Hackett (:bhackett) 2011-10-11 18:42:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda97b0f3e05
Comment 6 Marco Bonardo [::mak] 2011-10-12 03:19:16 PDT
https://hg.mozilla.org/mozilla-central/rev/dda97b0f3e05
Comment 7 Daniel Holbert [:dholbert] 2011-10-17 14:52:33 PDT
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?
Comment 8 Brian Hackett (:bhackett) 2011-10-17 14:57:40 PDT
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 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-17 15:04:38 PDT
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...
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-17 15:12:05 PDT
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 Daniel Holbert [:dholbert] 2011-10-17 15:13:48 PDT
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.
Comment 12 Brian Hackett (:bhackett) 2011-10-17 15:18:06 PDT
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).
Comment 13 Daniel Holbert [:dholbert] 2011-10-17 15:20:58 PDT
(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?)
Comment 14 Brian Hackett (:bhackett) 2011-10-17 15:23:52 PDT
Created attachment 567596 [details] [diff] [review]
remove JIT prefs from automation.py

Remove JIT prefs from automation.py
Comment 15 Brian Hackett (:bhackett) 2011-10-17 15:27:20 PDT
(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 David Mandelin [:dmandelin] 2011-10-18 13:04:03 PDT
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?
Comment 17 Brian Hackett (:bhackett) 2011-10-19 08:16:32 PDT
> 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.
Comment 18 Brian Hackett (:bhackett) 2011-10-19 08:30:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb5d09b48f03
Comment 19 Marco Bonardo [::mak] 2011-10-20 03:00:23 PDT
https://hg.mozilla.org/mozilla-central/rev/cb5d09b48f03

please check tree-management, one of these recent patches regressed V8

Note You need to log in before you can comment on or make changes to this bug.