javascript GC forced on window/document create/destroy: 12% of window open time

RESOLVED FIXED in mozilla0.9.7

Status

()

Core
DOM: Core & HTML
P1
normal
RESOLVED FIXED
18 years ago
4 years ago

People

(Reporter: Marko Macek, Assigned: jst)

Tracking

({dom0, perf})

Trunk
mozilla0.9.7
dom0, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

18 years ago
JS_GC is forces gc whenever the window/document is created and destroyed.

The calls are in functions SetNewDocument and SetupNewViewer.

This causes a big slowdown with lots of windows/documents and hurts in 
many operations.

GC shouldn't be called in fixed locations. I am not sure what the best strategy 
is. The current way is a scalability problem.

There are a few bugs that probably suffer from this: #38787, #39238, #39742, 
#39554, #28639
(Reporter)

Comment 1

18 years ago
Addinf perf, making bugs clickable:

bug 38787, bug 39238, bug 39742, bug 39554, bug 28639
Keywords: perf

Comment 2

18 years ago
I believe that GC is called here to ensure that JS contexts get cleaned up on 
window destruction, to ensure that object references which are held by JS objects 
are released. This is necessary to ensure proper window teardown.
(Reporter)

Comment 3

18 years ago
If that's the case it's probably a bug. Mozilla shouldn't rely on GC to run in a 
timely fashion.

Anyway, I commented out the calls to GC in those functions and no strange 
behavior was visible (I didn't check memory use).
The GC is called on document release and window destroy because those are 
guaranteed to make some garbage.  Maybe not enough to be worth collecting, but 
I'd like to see the numbers demonstrating a "big slowdown".  Note that the old 
"MozillaClassic" codebase ran GC there too.  Maybe it had fewer live objects to 
skip over, however.

Marko, can you provide quantitative information?

In any case, I don't think this bug belongs to rogerl.  I'm reassigning to jst, 
but he should feel free to push back or deprioritize till more hard data is in 
hand.

/be
Assignee: rogerl → jst
(Reporter)

Comment 5

18 years ago
bug 39554 (open new window) has a jprof output. If I don't GC there things are
25% faster (for second window) and much much faster when I have 30 or more
windows.

bug 38787 (open alt+L dialog) also has the jprof output (similiar to above)

bug 28639 (or a single window when many windows) also has the jprof output. This
operation is dominated by GC when there are many windows. Can take minutes (even
more than 10min easily when around 40-50 empty browser windows open)

(It seems that windows GC's much faster than linux for some reason, any ideas
why?)

The problem isn't just speed. It's scalability when there many windows open. I
browse with many windows open (open-in-new-window) so scalability is important.

I think JS_MaybeGC is possibly good solution. This should be changed soon so it
can be tested and tuned!

Comment 6

18 years ago
*** Bug 42785 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 7

18 years ago
Possibly also bug 27574 (Haven't profiled this one yet).

Comment 8

18 years ago
Changing component to DOM Level 0 to match new bug owner, jst
Component: Javascript Engine → DOM Level 0
QA Contact: pschwartau → desale

Updated

18 years ago
Blocks: 39742
(Assignee)

Comment 9

18 years ago
This is probably related to bug 39238, we should look into this for nsbeta3.
Status: NEW → ASSIGNED
Keywords: nsbeta3
(Assignee)

Updated

18 years ago
Target Milestone: --- → M18

Comment 10

18 years ago
This bug should be the root cause for bug 30660.
Blocks: 30660
I think some slowdown in JS_GC could be attributed to things leaking, as
mentioned in bug 39238.  Right now we leak entire XUL documents through leaked
JS roots in various ways (I have a few bugs on this that are fixed, and I'm
working on others).  As we leak more stuff the GC would be going through more
and more stuff, so it ought to get slower.

Comment 12

17 years ago
Marking nsbeta3+
Priority: P3 → P1
Whiteboard: [nsbeta3+]

Updated

17 years ago
Blocks: 49141
(Assignee)

Comment 13

17 years ago
*** Bug 39554 has been marked as a duplicate of this bug. ***

Comment 14

17 years ago
When opening or closing a window containing about:blank JS_ForceGC is called 4
times. When the window also contains frames it gets called even more (at least
one time extra per frame, didn't count).

The window creation gc calls are in nsDocShell::SetupNewViewer() and
GlobalWindowImpl::SetNewDocument(). The destruction gc calls are in
GlobalWindowImpl::SetNewDocument(), nsDocShell::Destroy() and in
nsJSContext::CallHandleEvent (when the context kungFuDeathGrip goes out of
scope).

So what can be done about this? Are these calls just "optimizations", or are
they needed for correct semantics? By just removing some calls and maybe moving
some it might be possible to get this to one GC per window creation/destruction.

Even then this takes some time. Opening 10 windows makes each GC take 100 msecs
on my dual PII  450MHz, and if i open a window with 100 frames (containing
about:blank) it takes over 500 msecs. Looking at the jprof profile shows that
almost all time is used when marking the gc roots, and especially hot is
gc_find_flags(). This functions (gc_find_flags) doesn't seem very fast, by some
smarter structures for arenas i think it can be made faster (binary search).

Comment 15

17 years ago
I've opened a new  bug (bug 49816) on some performance weaknesses of the
JavaScript GC.

Comment 16

17 years ago
Ok. This is pretty bad, even ignoring the gc performance problem with many live
objects (bug 49816) GC is just called *way* to much.

Opening a new window forces GC 4 times.
Closing a window forces GC 4 times.
Loading a page into a window (i.e. following a link) forces GC 3 times.
And this gets even worse when the loaded or destroyed page contains frames.

I hacked up a simple patch that minimized this to 1 GC on new window 1 GC on
document load and 2 on window closing, but this gave fatal assertions about
root_points_to_gcArenaPool(). This seems to be bug 41608 rearing its ugly head
again. It seems a globalwindow is leaked leading to it's root referenced not
being removed from the GC roots. 

Dunno why this is triggered by removing some GC:s though.
 

Comment 17

17 years ago
Created attachment 13485 [details] [diff] [review]
Fixes window open and document load to only GC once.

Comment 18

17 years ago
Ok. I've added a patch that fixes most of the unneccesary GC:s, but there is
still two done on window close (two JS_Contexts are being destroyed). But there
is just no way this patch will run stable until bug 43466 is fixed.
Depends on: 43466

Updated

17 years ago
No longer depends on: 43466
this impacts mac perf on new windows, adding keyword.
Keywords: nsmac1

Comment 20

17 years ago
PDT agrees P1

Comment 21

17 years ago
Ok, i run with the attached patch (id 13485) all the time now, and it works
well. It gives a good performance boost. Can someone please review it and try to
get it in? I think this is blocking a lot of other perf bugs too.

Comment 22

17 years ago
I confirm that the patch in attachment 13485 [details] [diff] [review] is a
great speedup for window-closing (though has no perceptible
effect on window-opening, should it?).

I haven't experienced any obvious problems with it yet.
jst, can you get that patch in, or say why not?  Thanks,

/be

Comment 24

17 years ago
Putting [pdtp1] in whiteboard
Whiteboard: [nsbeta3+] → [nsbeta3+][pdtp1]

Comment 25

17 years ago
Whats up with this patch? It's nsbeta3+ marked and finished. It just has to be
checked in. Why isn't it?
(Assignee)

Comment 26

17 years ago
The attached patch makes mozilla not GC at all when a window is closed, and
that's not acceptable. However, I have in my tree a slightly modified version
that will probably go in today, with any luck. I'm still running bloat tests to
make sure that reducing the GC calls won't cause too much bloat/leaks...

Comment 27

17 years ago
Huh?
By placing a printf() in js_ForceGC() I can see two GC on each closed window.
Here are the backtraces i get at the two GCs:

#0  js_ForceGC (cx=0x41b7db98) at /export/alex/mozilla/js/src/jsgc.c:708
#1  0x40174051 in js_DestroyContext (cx=0x41b7db98, gcmode=JS_FORCE_GC) at
/export/alex/mozilla/js/src/jscntxt.c:219
#2  0x4016b80e in JS_DestroyContext (cx=0x41b7db98) at
/export/alex/mozilla/js/src/jsapi.c:801
#3  0x40476cc0 in nsJSContext::~nsJSContext (this=0x41bbee68, __in_chrg=3) at
/export/alex/mozilla/dom/src/base/nsJSEnvironment.cpp:367
#4  0x40477064 in nsJSContext::Release (this=0x41bbee68) at
/export/alex/mozilla/dom/src/base/nsJSEnvironment.cpp:375
#5  0x4047a3df in nsJSContext::CallEventHandler (this=0x41bbee68,
aTarget=0x42089ed0, aHandler=0x42089ed8, argc=1, argv=0xbfffbb48, 
    aBoolResult=0xbfffbb4c, aReverseReturnResult=0) at
../../../dist/include/nsCOMPtr.h:489
#6  0x404d684c in nsJSEventListener::HandleEvent (this=0x42038788,
aEvent=0x420ef50c) at
/export/alex/mozilla/dom/src/events/nsJSEventListener.cpp:154
#7  0x413666f0 in nsEventListenerManager::HandleEventSubType (this=0x42038730,
aListenerStruct=0x420387c0, aDOMEvent=0x420ef50c, 
    aCurrentTarget=0x41d879d0, aSubType=8, aPhaseFlags=7) at
/export/alex/mozilla/layout/events/src/nsEventListenerManager.cpp:788
#8  0x413683ec in nsEventListenerManager::HandleEvent (this=0x42038730,
aPresContext=0x41baf7c8, aEvent=0xbfffc744, aDOMEvent=0xbfffbf3c, 
    aCurrentTarget=0x41d879d0, aFlags=7, aEventStatus=0xbfffc6fc) at
/export/alex/mozilla/layout/events/src/nsEventListenerManager.cpp:1666
#9  0x406fce0a in nsXULElement::HandleDOMEvent (this=0x41d879c8,
aPresContext=0x41baf7c8, aEvent=0xbfffc744, aDOMEvent=0xbfffbf3c, aFlags=1, 
    aEventStatus=0xbfffc6fc) at
/export/alex/mozilla/rdf/content/src/nsXULElement.cpp:3306
#10 0x40761b8a in nsXULKeyListenerImpl::HandleEventUsingKeyset (this=0x41bad178,
aKeysetElement=0x41bad124, aKeyEvent=0x420ef490, 
    aEventType=eKeyPress, aDocument=0x41b47d8c, aHandledFlag=@0xbfffd0d0) at
/export/alex/mozilla/rdf/content/src/nsXULKeyListener.cpp:1718
#11 0x4075b884 in nsXULKeyListenerImpl::LocateAndExecuteKeyBinding
(this=0x41bad178, aEvent=0x420ef490, aEventType=eKeyPress, aDocument=0x41b47d8c, 
    aHandledFlag=@0xbfffd0d0) at
/export/alex/mozilla/rdf/content/src/nsXULKeyListener.cpp:1358
#12 0x40757fb3 in nsXULKeyListenerImpl::DoKey (this=0x41bad178,
aKeyEvent=0x420ef494, aEventType=eKeyPress)
    at /export/alex/mozilla/rdf/content/src/nsXULKeyListener.cpp:649
#13 0x40757811 in nsXULKeyListenerImpl::KeyPress (this=0x41bad178,
aKeyEvent=0x420ef494)
    at /export/alex/mozilla/rdf/content/src/nsXULKeyListener.cpp:593
#14 0x41367378 in nsEventListenerManager::HandleEvent (this=0x41b48ba0,
aPresContext=0x422137a8, aEvent=0xbffff75c, aDOMEvent=0xbffff360, 
    aCurrentTarget=0x41b47da0, aFlags=2, aEventStatus=0xbffff688) at
/export/alex/mozilla/layout/events/src/nsEventListenerManager.cpp:1114

#0  js_ForceGC (cx=0x420fde60) at /export/alex/mozilla/js/src/jsgc.c:708
#1  0x40174051 in js_DestroyContext (cx=0x420fde60, gcmode=JS_FORCE_GC) at
/export/alex/mozilla/js/src/jscntxt.c:219
#2  0x4016b80e in JS_DestroyContext (cx=0x420fde60) at
/export/alex/mozilla/js/src/jsapi.c:801
#3  0x40476cc0 in nsJSContext::~nsJSContext (this=0x420fde20, __in_chrg=3) at
/export/alex/mozilla/dom/src/base/nsJSEnvironment.cpp:367
#4  0x40477064 in nsJSContext::Release (this=0x420fde20) at
/export/alex/mozilla/dom/src/base/nsJSEnvironment.cpp:375
#5  0x4048e45b in GlobalWindowImpl::HandleDOMEvent (this=0x420fdd40,
aPresContext=0x422137a8, aEvent=0xbffff75c, aDOMEvent=0xbffff360, aFlags=2, 
    aEventStatus=0xbffff688) at ../../../dist/include/nsCOMPtr.h:489
#6  0x41690188 in nsDocument::HandleDOMEvent (this=0x4221c3d0,
aPresContext=0x422137a8, aEvent=0xbffff75c, aDOMEvent=0xbffff360, aFlags=2, 
    aEventStatus=0xbffff688) at
/export/alex/mozilla/layout/base/src/nsDocument.cpp:3040
#7  0x416c3088 in nsGenericElement::HandleDOMEvent (this=0x4221c7bc,
aPresContext=0x422137a8, aEvent=0xbffff75c, aDOMEvent=0xbffff360, aFlags=1, 
    aEventStatus=0xbffff688) at
/export/alex/mozilla/layout/base/src/nsGenericElement.cpp:1449
#8  0x414429e3 in nsHTMLHtmlElement::HandleDOMEvent (this=0x4221c7a8,
aPresContext=0x422137a8, aEvent=0xbffff75c, aDOMEvent=0x0, aFlags=1, 
    aEventStatus=0xbffff688) at
/export/alex/mozilla/layout/html/content/src/nsHTMLHtmlElement.cpp:185
#9  0x413d899d in PresShell::HandleEventInternal (this=0x42214258,
aEvent=0xbffff75c, aView=0x41b247c0, aStatus=0xbffff688)
    at /export/alex/mozilla/layout/html/base/src/nsPresShell.cpp:4182
#10 0x413d86ca in PresShell::HandleEvent (this=0x42214258, aView=0x41b247c0,
aEvent=0xbffff75c, aEventStatus=0xbffff688, aForceHandle=0, 
    aHandled=@0xbffff61c) at
/export/alex/mozilla/layout/html/base/src/nsPresShell.cpp:4117
#11 0x41a43b89 in nsView::HandleEvent (this=0x41b247c0, event=0xbffff75c,
aEventFlags=8, aStatus=0xbffff688, aForceHandle=0, aHandled=@0xbffff61c)
    at /export/alex/mozilla/view/src/nsView.cpp:366
#12 0x41a43b2e in nsView::HandleEvent (this=0x420e8410, event=0xbffff75c,
aEventFlags=8, aStatus=0xbffff688, aForceHandle=0, aHandled=@0xbffff61c)
    at /export/alex/mozilla/view/src/nsView.cpp:350
#13 0x41a43b2e in nsView::HandleEvent (this=0x41b9cfa0, event=0xbffff75c,
aEventFlags=28, aStatus=0xbffff688, aForceHandle=1, aHandled=@0xbffff61c)
    at /export/alex/mozilla/view/src/nsView.cpp:350
#14 0x41a4e2be in nsViewManager2::DispatchEvent (this=0x4220fda8,
aEvent=0xbffff75c, aStatus=0xbffff688)
    at /export/alex/mozilla/view/src/nsViewManager2.cpp:1427

They are called at the destruction of the JS contexts for the webshell and the
xul window (or something like that).
(Assignee)

Comment 28

17 years ago
Yes, you're right, my bad. I'll try to land something tonight, and thanks for
the patch.

Comment 29

17 years ago
Actually. Having a printf in js_ForceGC() gives quite some insight into how
often the GC is called. It's a humbling experience, but the GC should be much
faster now since brendan checked in the gc_find_flags patch.
(Assignee)

Comment 30

17 years ago
The attached patch is now checked in but nsJSContext::GC is still being called
once when opening a new window and I'd like to get rid of that too. I'll
investigate...

Comment 31

17 years ago
Created attachment 14685 [details] [diff] [review]
Here is a patch for interested parties that prints out the time of each js_ForceGC()

Comment 32

17 years ago
Marking nsbeta3-.  Nominating for rtm for the remaining part of Johnny's fix.
Keywords: rtm
Whiteboard: [nsbeta3+][pdtp1] → [nsbeta3-][pdtp1]

Comment 33

17 years ago
According to Johnny, the remaining part of the fix is not gonna give us much of
a performance improvement.  Marking rtm-.
Whiteboard: [nsbeta3-][pdtp1] → [nsbeta3-][pdtp1][rtm-]

Comment 34

17 years ago
Marking future...
Target Milestone: M18 → Future
Keywords: dom0
Created attachment 34681 [details] [diff] [review]
avoid GC for window open
r/sr=brendan@mozilla.org, so obvious in hindsight!

/be
(Assignee)

Comment 37

17 years ago
Fine with me :-) r/sr=jst
OK, that's checked in.  I think we still have the issue that we do a GC on the
transition from 'about:blank' to the homepage, but I'd have to double-check on
that.  It would also be nice to delay GC until after the new page has been
presented to the user...
That was backed out, for good reason, since it stopped the GC on page-to-page
transitions.  (However, the real reason that it was backed out was that it
caused extra doc viewers to be lying around that still painted -- something for
which the real fix is similar to the patch I put on bug 80203.)  Oops... and sorry!

Perhaps think the real fix here is to test |mDocument| but *not* |aDocument|,
and then avoid the extra GC when closing windows at the site where it happens
rather than in |SetNewDocument|?

Comment 40

17 years ago
What sort of testing preceded this checkin?  The problem that resulted from this
checkin seems like the kind of thing sr= is supposed to avoid.  Was this really
an obscure result that was difficult to predict from review or find through unit
testing??
selmer, I hope you weren't just venting with rhetorical questions, because while
this bug cost some people too much time, it was certainly not a blocker, and it
did not need to stop the whole Netscape-contributors-to-Mozilla world.  At most,
a few Gecko experts should have been brought in to diagnose it quickly.  This
episode also reveals how a worse underlying bug than the commercial smoketest
blocker that was filed on Monday can be papered over too many times, without
being fixed.

>What sort of testing preceded this checkin?

dbaron can say exactly what he tested in addition to precheckin tests, if he
cares to -- but is it not the case that the immediate breakage was "only" (not
to minimize the damage, but to show that the effect was narrow and confined to
commercial) to IM migration, and that that symptom was reported *only* via a
bugscape report?  It's not as if this patch broke daily non-commercial Mozilla
smoketests, or the tree would never have opened on Monday.

>The problem that resulted from this checkin seems like the kind of thing sr=
>is supposed to avoid.

Super-reviewers are fallible, and I failed big-time here (the nulling of
mDocument before the test is even visible in the diff -u!  "So obvious", I'm
cringing).  Sorry about that.

But why single out super-review?  It was not promised as a cure-all, and you
know it.  It's not even a testing process, we have those already.  At most, a
super-reviewer can ask for more testing if the patch and the person submitting
it (important in this case: dbaron's excellent reputation continues, in spite of
this regression) indicate more testing is needed.

Super-review and review blunders to the contrary notwithstanding, what SR excels
at, certainly compared to QA, is improving deep code quality and fixing design
flaws or other kinds of underlying bugs that cause multiple and recurring
symptoms.  For example, the terrible Gecko code that performs vital semantic
"page teardown" actions from a document viewer destructor, which is a red flag
to any super-reviewer versed in ref-counting and garbage collection, and which
is well-known to more than few Mozilla reviewers (it originated long before
super-review).

QA will not find such a problem in the code.  QA will find only its symptoms (a
valuable service, to be sure), which can be patched around and papered over --
and which have been papered over for too long, at costs far greater than the
cost that dbaron's regression incurred.

>Was this really an obscure result that was difficult to predict from review

Which result do you mean?  IM migration in commercial builds breaking?

The reviewers and patch submitter missed something in the code that breaks page
to page GC, which is bad in itself (although we run GC too often anyway, so the
memory effect was not obvious), and also (due to that Gecko document viewer
destruction requirement botch on page change) breaks Gecko painting in certain
cases.

IM migration breaking was indeed an obscure consequence, predicated on JS
peculiar to profile migration XUL holding the last ref to a doc viewer for an
unloaded page.  But the reviewers should have caught the bad code, whether or
not they predicted every last symptom.

Why do you ask this question?  It's wrong-headed, because the answer is "yes,
the detailed symptom really was an obscure result that was hard to predict from
review."  But that does not excuse the faulty reviewers, especially me!  It is
emphatically not necessary for super-reviewers to predict every bad surface
effect of a change.  The inspectable result of breaking page-to-page GC was bad
enough, never mind the entanglement with the docviewer lifetime botch in Gecko.
 
>or find through unit testing??

What unit testing?  "Works in viewer" :-).  There is no DOM Unit Test, AFAIK.

Please note again that since dbaron's checkin, nothing held the Mozilla tree
closed.  The precheckin tests passed for everyone who ran them on builds updated
to include his checkin.  The world did not end.  Only IM migration broke, as far
as anyone could tell, to the best of my knowledge.  And dbaron@fas.harvard.edu
does not have access to bugscape.

/be

Comment 42

17 years ago
Brendan, my questions were not rhetorical, merely poorly stated due to some
frustration.  The heart of the question is just to gather data about what
happened since this became part of a larger problem.  I know super review is
neither perfect nor a cure-all.  That part of the question was prompted by
dbaron's last comment about what was wrong. 

My question about testing was also aimed at dbaron's last comment describing
what was wrong, not the IM impact.  I'm aware that dbaron can't be expected to
know anything about Netscape commercial impacts.  (Although a few people have
volunteered to help non-Netscaper's find that out.)

So, how does someone who is attempting a fix such as this (to the timing of
running the GC) objectively determine whether things got better?  I was hoping
there was something more structured than what actually exists.  Maybe some
existing stuff like jrgm's page loading suite would be useful?  (I don't know,
just asking :-)
Blocks: 83421
In bug 49141, I've found that removing the GC() call in SetNewDocument() cuts
window open time (for 20 windows in the 49141 testcase) by about 12%.  In fact,
hacking it to be MaybeGC also saves 12%.  If, as dbaron guesses, we no longer
have a requirement to GC on page transitions since bug 80203 is fixed, we should
look at removing this, or at least making it a MaybeGC() call.

As a somewhat safer alternative that wins us considerably less, jst has a patch
that does GC except on open window.

I think we should yank it and resolve or fix any issues it brings up.

I consider having GC be required to avoid bugs to be problematic.
 
Nominating for 0.9.6.  This is a 12% win for window open.  Let's find/fix
anything that requires GC on window open/close (if anything does) and get that
12% win.
Keywords: mozilla0.9.6

Comment 45

16 years ago
Removing adequated PDT grafitti.
Whiteboard: [nsbeta3-][pdtp1][rtm-]

Comment 46

16 years ago
If this is 12% of new window, what percent of page load is it?
Summary: javascript GC forced on window/document create/destroy → javascript GC forced on window/document create/destroy: 12% of window open time

Comment 47

16 years ago
johnny, this would be a good one to fix within next 2 milestones.  :-)
setting to 0.9.7
Target Milestone: Future → mozilla0.9.7

Comment 48

16 years ago
Since window opening speed has improved a bit (but noticeably, thanks guys!) for
Linux since dbaron's post on 9/17, its possible that the win from this may be
even larger than 12% now.  That'd make this the largest single win for this
nagging problem ever (or at least since I started using Mozilla for daily
browsing at 0.8)  I for one am really looking forward to seeing this one land!
(Assignee)

Comment 49

16 years ago
I agree that this is something that should be fixed, but I find it extremely
hard t obe lieve that this would get us a 12% speed improvment when opening new
windows, maybe once you have a large number of windows open (which causes every
GC run to take more time), but with only a few windows open I higly doubt it.

I'm looking into moving the actual JS_GC() calls off on a timer so that we don't
GC immediately when opening or closing new windows.

Comment 50

16 years ago
We just need to get the GC happening after the onload of the newly opened
window, so that the window has shown before the GC happens.
(Assignee)

Comment 51

16 years ago
Does delaying GC 2 seconds seem like way too long?
(Assignee)

Comment 52

16 years ago
Hyatt, GC actually happens as a new document comes *in*, i.e. when we get the
first responce from the server and we know what kind of document to create (i.e.
when the old document is torn down). I have a patch that in stead of GC'ing at
that point we'll fire off a low priority timer (unless there already is a GC
timer scheduled) that will do the GC 2 seconds from that time. If the page takes
more than 2 seconds to load it means we'll GC in the middle of loading the page.
(Assignee)

Comment 53

16 years ago
Created attachment 58678 [details] [diff] [review]
Delay GC 2 seconds...

This patch makes us do the GC 2 seconds after the document is torn down, and if
we're asked to do more GC's during those 2 seconds we'll ignore those GC
requests (since we'll do one once the timer fires anyway). Also, the first time
we're asked to GC we'll delay that GC 10 seconds, this should make us GC far
less on startup, which means faster startup.
(Reporter)

Comment 54

16 years ago
The JS_GC should really be JS_MaybeGC or something else that can be tuned.

It doesn't really help if the GC is just delayed for 2 seconds unless the 2
seconds means browser idle-time.
Doesn't this have the risk of causing leaks in the case where you don't fire the
timer because there's already one, since you don't clear the newborns on that
context?  Do we need a JS_ClearNewborns API?

Comment 56

16 years ago
Marko, it does help if the XUL window shows within 2 secs, since the GC will
then no longer be part of new window time (since it will occur after the window
has shown).
jst: nit, why not use NS_IMPL_ISUPPORTS3?

-NS_IMPL_ISUPPORTS2(nsJSContext, nsIScriptContext, nsIXPCScriptNotify)
+// QueryInterface implementation for nsJSContext
+NS_INTERFACE_MAP_BEGIN(nsJSContext)
+  NS_INTERFACE_MAP_ENTRY(nsIScriptContext)
+  NS_INTERFACE_MAP_ENTRY(nsIXPCScriptNotify)
+  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIScriptContext)
+NS_INTERFACE_MAP_END

dbaron, jst: there is a JS_ClearNewbornRoots API now -- others needed it too and
epstein@tellme.com contributed a patch.

/be
(Assignee)

Comment 58

16 years ago
Created attachment 58782 [details] [diff] [review]
Clear newborn roots if we don't fire a GC timer.
(Assignee)

Comment 59

16 years ago
Brendan, I don't do NS_IMPL_ISUPPORTS_n_ macros :-) They're a pain if you want
to set breakpoints in addref and release (which is sometimes quite useful), and
likewise you can't see what happens in QI at all (in the debugger) if you use
those macros. IMO the QI maps and the individual addref and release macros are
way cleaner and makes much more sense.
jst: what debugger are you using?  I can set breakpoints by qualified method
name in gdb, MSVC, etc. -- no matter how macro-ized the declaration of the
method.  I don't see the problem, unless you mean that you want to step through
QI and watch the source line move from one NS_INTERFACE_MAP_ENTRY to the next. 
Cleanliness favors brevity, and the NS_IMPL_ISUPPORTS* macros expand to the
sequence you are verbosely open-coding.

Cc'ing dougt and shaver, who may admonish you about levels of API frozen-ness
(e.g., NS_IMPL_* is frozen but NS_INTERFACE_MAP* is not) -- or they may just
tell me to buzz off!

/be
I don't care if jst uses non-frozen APIs (the things produced by the interface
maps will always be binary compatible, I think -- AddRef and Release aren't
likely to change -- if not always source compatible), but it's on his head to
update if the way we implement NS_IMPL_ISUPPORTSn in the future, perhaps to make
nsIClassInfo more widely supported.

(And really, you find it a lot better to step through the 7-line macro?  You
still can't see what it's doing, and you already know which interface it's
asking for by looking at the caller.  What brendan said about setting
breakpoints, too; what sort of bass-ackwards debugger are you using that won't
let you set a breakpoint on a macro-define nsFoo::AddRef?)

Comment 62

16 years ago
Either way.  if I was reviewing the code, I would ask that you use the other
macro's just because that is what everyone else is doing - when in Rome.  

shaver, i disagree on one important point.  If we update NS_IMPL_ISUPPORTSn to
do something new, jst is the last person that will be blamed to fix any
bustage... It is going to be me or you or whoever was brave enough to make such
a change.
(Assignee)

Comment 63

16 years ago
MS's bass-ackwards debugger doesn't provide a trivial way of setting breakpoints
in methods hidden in macros, it's doable but it's less trivial than simply
pointing at the macro and pressing F9.

Also, if you look at *every* DOM class, they use these QI maps and
AddRef/Release macros simply because they're more flexible when using classinfo,
dynamic XBL implemented interfaces n' such stuff. I'm sorry, but I haven't heard
compelling reasons for not using them here yet. If you change the
NS_IMPL_ISUPPORTSn macro's in an incompatible way, you'll break the world,
you'll notice...

And, again, I personally hate the NS_IMPL_ISUPPORTSn macros, they don't scale,
they're not flexible at all. I don't see them giving us anything over the
separate QI map and AddRef/Release macros, and yes, I do disagree with us
pushing people to use the NS_IMPL_ISUPPORTSn macros over the more flexible ones.

I don't want to start a jihad over which style to use in this particular case,
so if you feel really really strongly about which ones to use, convince me and
I'll undo that change.

Comment 64

16 years ago
use either.  It really does not matter that much.  
One reason to encourage use of the NS_IMPL_ISUPPORTSn macros is that people
using the map entry risk leaving off the nsISupports entry.  Admittedly, out of
203 uses in the SeaMonkey module, only 4 have done so (inBitmapDecoder,
nsMsgFilePostHelper, nsIconDecoder, and nsHttpConnection).  That is almost 2% of
users, though...
dbaron's point is akin to the general reason we macro-ize common repeated code
sequences.  Sure, you don't save on compiled code, but you don't have to type so
much and court data-entry/cut-paste/typo/brain-fart errors.  You also get the
benefit of abstraction: once you've chosen NS_IMPL_ISUPPORTSn, you don't need to
worry about how it's implemented.  This is compelling reason #1.

CR#2 is brevity, which seems close to the soul of cleanliness.  jst, you
asserted that the longer form was way cleaner, but I don't buy it if clean ==
concise.  I do see your point if you want macro calls to expand to syntacticly
complete units -- in that light, PR_BEGIN_MACRO and PR_END_MACRO are unclean. 
But so are the INTERFACE_MAP macros!

In MSVC, typing control-B and then entering the method name worksforme.  This
inconvenience is not a compelling reason to ignore CR#1 and CR#2.

I realize this seems like a small point, but it comes up during super-review,
and the super-reviewees need to hear a consistent story that "scales" over 203
uses without a 2% error rate.  jst, I think your "they don't scale" complaint
applies more to this real-world error evidence that dbaron cites, than to any
macro proliferation required for more interfaces, future CI/THREADSAFE/etc.
variations.  Let's write code once, not spread the error-prone task of writing
it all over the place.

Cc'ing jband for fun.

/be
Comment on attachment 58782 [details] [diff] [review]
Clear newborn roots if we don't fire a GC timer.

sr=brendan@mozilla.org.  I'm willing to get this into the trunk for a try-out. 
If waiting 2 seconds breaks something or leads to too much transient bloat,
let's find that out the only way I know how.

/be
Attachment #58782 - Flags: superreview+
Comment on attachment 58782 [details] [diff] [review]
Clear newborn roots if we don't fire a GC timer.

r=bzbarsky
Attachment #58782 - Flags: review+
(Assignee)

Comment 69

16 years ago
Yeah, the error prone-ness of the map version of these macros is kinda bad, so
that's a fair reason to not advocate their use. However, we could add code in
nsISupportsImpl.h that would assert if a QI method is called on an object whose
QI doesn't support nsISupports, and that would cover that problem. Should I hack
something up?

I never said the debugging aspects of these macros were a big deal (when I said
it was a pain to debug when using the oneline macros, I meant a really small
pain :-) ), but they do make you do more when setting a breakpoint in the
methods hidden by these macros, than you need to with the separated macros.
Again, this is not a big deal.

I guess my biggest reason for using the map version is for consistency with 99%
of the DOM code, where it's just not practical to use the NS_IMPL_ISUPPORTSn
macros since I haveto deal with classinfo in every class, and some XBL QI
aggregation here n' there. So I do always have to worry about how these things
are implemented, so the more that's hidden behind macros the harder my life gets.

One more thing that's a bit lame about the NS_IMPL_ISUPPORTSn macros is that
they don't let you decide where in the map nsISupports goes, which in some
cases, admittedly only in edge cases, does matter for performance reasons.

Anyway, the fix is checked in (using the MAP macros, so sue me ;-) ). Marking
FIXED, lets re-open this if this turns out to not be a good solution for this
problem...
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 70

16 years ago
>the first time we're asked to GC we'll delay that GC 10 seconds, this should 
>make us GC far less on startup, which means faster startup.

Barrett's launch perf posts in netscape.public.mozilla.performance didn't show 
an improvement between 11-26 and 11-27.  Should I be worried, or was the fix 
not expected to improve startup perf much?

How much improvement did you get for window open time?  I don't see a graph for 
new window perf on the newsgroup, which is frustrating because it's the aspect 
of performance I'm most interested in.
(Assignee)

Comment 71

16 years ago
I know we GC *less* on startup, but that doesn't necessarily mean a measureable
improvment in startup performance. As for new window performance there should be
an improvment, at least when opening a new window when a large number of windows
are already open (since the more windows we have, the more expensive a GC run is).
(Assignee)

Comment 72

16 years ago
*** Bug 77236 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.