Last Comment Bug 698201 - Remove tracer
: Remove tracer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: David Anderson [:dvander]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 693815 705380 706611 707341 707351 709792 710481 710486 714894 751280
Blocks: 647628 692838 704356 705168 705356 705401 714052
  Show dependency treegraph
 
Reported: 2011-10-29 08:45 PDT by Tom Schuster [:evilpie]
Modified: 2012-05-02 11:46 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: remove major files (2.27 MB, patch)
2011-11-09 13:53 PST, David Anderson [:dvander]
dmandelin: review+
Details | Diff | Splinter Review
part 2: remove imacros (38.10 KB, patch)
2011-11-09 13:54 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review
part 3: remove traceable natives from js/src (97.57 KB, patch)
2011-11-09 13:55 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review
part 4: remove everything def'd under JS_TRACER (55.43 KB, patch)
2011-11-09 13:56 PST, David Anderson [:dvander]
bhackett1024: review+
Details | Diff | Splinter Review
part 5: remove structures, functions (88.27 KB, patch)
2011-11-09 13:57 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review
part 6: xpconnect changes (42.80 KB, patch)
2011-11-09 14:00 PST, David Anderson [:dvander]
bobbyholley: review+
Details | Diff | Splinter Review
part 7: remove browser prefs (7.52 KB, patch)
2011-11-09 14:02 PST, David Anderson [:dvander]
gal: review+
gal: superreview+
Details | Diff | Splinter Review
Remove some left-overs from JM (20.96 KB, patch)
2011-11-23 08:38 PST, Tom Schuster [:evilpie]
dvander: review+
Details | Diff | Splinter Review

Description User image Tom Schuster [:evilpie] 2011-10-29 08:45:55 PDT
The time has come.

tom@tom-linux:~/inbound/js/src$ hg log jstracer.cpp | grep -i "bug" -c
1474

Brain thinks dvander has a patch for this?
Comment 1 User image Cameron Kaiser [:spectre] 2011-10-30 08:21:03 PDT
Wait, I thought this wasn't happening until 11?
Comment 2 User image Tom Schuster [:evilpie] 2011-10-30 08:26:49 PDT
This is just for bookkeeping. It is supposed to be done after FF10 merges to Aurora.
Comment 3 User image Nicholas Nethercote [:njn] 2011-10-30 20:34:26 PDT
Note: don't forget the tracer-related stuff in xpconnect/src/XPCJSRuntime.cpp.
Comment 4 User image David Anderson [:dvander] 2011-11-09 13:53:17 PST
Created attachment 573310 [details] [diff] [review]
part 1: remove major files

Remove nanojit/*, jstracer*, lirasm/*, and friends.

This queue is based on top of Bill's write barriers patch. I'll send it to try soon - the browser builds locally and tests pass.
Comment 5 User image David Anderson [:dvander] 2011-11-09 13:54:01 PST
Created attachment 573311 [details] [diff] [review]
part 2: remove imacros
Comment 6 User image David Anderson [:dvander] 2011-11-09 13:55:50 PST
Created attachment 573313 [details] [diff] [review]
part 3: remove traceable natives from js/src
Comment 7 User image David Anderson [:dvander] 2011-11-09 13:56:49 PST
Created attachment 573314 [details] [diff] [review]
part 4: remove everything def'd under JS_TRACER
Comment 8 User image David Anderson [:dvander] 2011-11-09 13:57:58 PST
Created attachment 573315 [details] [diff] [review]
part 5: remove structures, functions
Comment 9 User image David Anderson [:dvander] 2011-11-09 14:00:14 PST
Created attachment 573316 [details] [diff] [review]
part 6: xpconnect changes
Comment 10 User image David Anderson [:dvander] 2011-11-09 14:02:52 PST
Created attachment 573317 [details] [diff] [review]
part 7: remove browser prefs
Comment 11 User image Luke Wagner [:luke] 2011-11-09 14:36:08 PST
Comment on attachment 573311 [details] [diff] [review]
part 2: remove imacros

r++++++ would review again
Comment 12 User image Luke Wagner [:luke] 2011-11-09 14:43:38 PST
Comment on attachment 573315 [details] [diff] [review]
part 5: remove structures, functions

I'd be inclined to just remove JSOPTION_JIT.
Comment 13 User image Brian Hackett (:bhackett) 2011-11-09 14:44:38 PST
Comment on attachment 573314 [details] [diff] [review]
part 4: remove everything def'd under JS_TRACER

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

What is the plan for the deprecated API bits like JSOPTION_JIT and JS_SetGCParameterForThread (and no-ops below it)?
Comment 14 User image Nicholas Nethercote [:njn] 2011-11-09 14:57:13 PST
Can you run "diffstats" on the combined patches? :)
Comment 15 User image Andreas Gal :gal 2011-11-09 15:18:11 PST
Comment on attachment 573317 [details] [diff] [review]
part 7: remove browser prefs

r=me, the tracer shall rise again, if it makes sense down the road
Comment 16 User image Bobby Holley (:bholley) (busy with Stylo) 2011-11-09 15:55:33 PST
Comment on attachment 573316 [details] [diff] [review]
part 6: xpconnect changes

\o/
Comment 17 User image David Anderson [:dvander] 2011-11-09 19:41:19 PST
(In reply to Brian Hackett from comment #13)
> What is the plan for the deprecated API bits like JSOPTION_JIT and
> JS_SetGCParameterForThread (and no-ops below it)?

I'll get rid of JSOPTION_JIT. Do you think I should get rid of the other ones as well? It doesn't seem like we have plans for them.

(In reply to Nicholas Nethercote [:njn] from comment #14)
> Can you run "diffstats" on the combined patches? :)

 350 files changed, 126 insertions(+), 67643 deletions(-)
Comment 18 User image :Ms2ger (⌚ UTC+1/+2) 2011-11-14 04:07:28 PST
Ship it, I say!
Comment 19 User image :Ms2ger (⌚ UTC+1/+2) 2011-11-15 02:41:36 PST
... Once it passes try.

https://tbpl.mozilla.org/?tree=Try&rev=7e79a6ec0b0c

doesn't look good.
Comment 20 User image Nicholas Nethercote [:njn] 2011-11-15 14:15:51 PST
Actually, since the tracer was already turned off, removing it from the codebase won't make any difference to memory usage.  So I've removed the MemShrink tag.
Comment 21 User image Nicholas Nethercote [:njn] 2011-11-15 14:16:40 PST
And by "turned off" I mean "not built by default".
Comment 24 User image Tom Schuster [:evilpie] 2011-11-23 06:22:29 PST
\o/

Why is "jstracer.cpp" still there?
Comment 25 User image Andrew McCreight [:mccr8] 2011-11-23 06:36:12 PST
jandem pointed out in IRC that the removal of jstracer.cpp is present in the reviewed patch attached to the bug, but not in the patch landed on inbound.
Comment 26 User image David Anderson [:dvander] 2011-11-23 06:59:38 PST
Thanks, I'm not sure how that happened but I pushed a follow-up fix:

http://hg.mozilla.org/integration/mozilla-inbound/rev/4864134dad4e
Comment 27 User image Tom Schuster [:evilpie] 2011-11-23 08:38:07 PST
Created attachment 576516 [details] [diff] [review]
Remove some left-overs from JM
Comment 28 User image David Anderson [:dvander] 2011-11-23 09:03:37 PST
Comment on attachment 576516 [details] [diff] [review]
Remove some left-overs from JM

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

Looks good, but let's do follow-up bugs for future tracer-ectomies.

::: js/src/methodjit/MethodJIT-inl.h
@@ +90,5 @@
>      JITScriptStatus status = script->getJITStatus(fp->isConstructing());
>      if (status == JITScript_Invalid)
>          return Compile_Abort;
>      if (status == JITScript_None && !cx->hasRunOption(JSOPTION_METHODJIT_ALWAYS)) {
> +        if ((cx->typeInferenceEnabled())

Nit: extra parens
Comment 29 User image Matt Brubeck (:mbrubeck) 2011-11-23 10:13:27 PST
Follow-up to remove jstracer.cpp merged to trunk:
https://hg.mozilla.org/mozilla-central/rev/4864134dad4e
Comment 30 User image Zio_Fede 2011-11-24 22:34:54 PST
There are still the javascript.options.jitprofiling about:config voices. Shouldn't they be removed?
Comment 31 User image :Ms2ger (⌚ UTC+1/+2) 2011-11-25 00:54:58 PST
(In reply to Zio_Fede from comment #30)
> There are still the javascript.options.jitprofiling about:config voices.
> Shouldn't they be removed?

Followup, please. (But doesn't that apply to other jits as well?)
Comment 32 User image Ryan VanderMeulen [:RyanVM] 2011-11-25 14:49:32 PST
My patch in bug 705356 removes the profiling options as well. Let's use that bug for that discussion.

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