Closed Bug 42321 Opened 26 years ago Closed 24 years ago

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

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: Marko.Macek, Assigned: jst)

References

Details

(Keywords: dom0, perf)

Attachments

(5 files)

Fixes window open and document load to only GC once.
1.11 KB, patch
Details | Diff | Splinter Review
Here is a patch for interested parties that prints out the time of each js_ForceGC()
862 bytes, patch
Details | Diff | Splinter Review
avoid GC for window open
618 bytes, patch
Details | Diff | Splinter Review
Delay GC 2 seconds...
11.79 KB, patch
Details | Diff | Splinter Review
Clear newborn roots if we don't fire a GC timer.
11.83 KB, patch
bzbarsky
: review+
brendan
: superreview+
Details | Diff | Splinter Review
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
Addinf perf, making bugs clickable: bug 38787, bug 39238, bug 39742, bug 39554, bug 28639
Keywords: perf
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.
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
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!
*** Bug 42785 has been marked as a duplicate of this bug. ***
Possibly also bug 27574 (Haven't profiled this one yet).
Changing component to DOM Level 0 to match new bug owner, jst
Component: Javascript Engine → DOM Level 0
QA Contact: pschwartau → desale
Blocks: 39742
This is probably related to bug 39238, we should look into this for nsbeta3.
Status: NEW → ASSIGNED
Keywords: nsbeta3
Target Milestone: --- → M18
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.
Marking nsbeta3+
Priority: P3 → P1
Whiteboard: [nsbeta3+]
Blocks: 49141
*** Bug 39554 has been marked as a duplicate of this bug. ***
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).
I've opened a new bug (bug 49816) on some performance weaknesses of the JavaScript GC.
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.
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
No longer depends on: 43466
this impacts mac perf on new windows, adding keyword.
Keywords: nsmac1
PDT agrees P1
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.
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
Putting [pdtp1] in whiteboard
Whiteboard: [nsbeta3+] → [nsbeta3+][pdtp1]
Whats up with this patch? It's nsbeta3+ marked and finished. It just has to be checked in. Why isn't it?
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...
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).
Yes, you're right, my bad. I'll try to land something tonight, and thanks for the patch.
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.
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...
Marking nsbeta3-. Nominating for rtm for the remaining part of Johnny's fix.
Keywords: rtm
Whiteboard: [nsbeta3+][pdtp1] → [nsbeta3-][pdtp1]
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-]
Marking future...
Target Milestone: M18 → Future
Keywords: dom0
r/sr=brendan@mozilla.org, so obvious in hindsight! /be
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|?
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
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 :-)
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
Removing adequated PDT grafitti.
Whiteboard: [nsbeta3-][pdtp1][rtm-]
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
johnny, this would be a good one to fix within next 2 milestones. :-) setting to 0.9.7
Target Milestone: Future → mozilla0.9.7
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!
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.
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.
Does delaying GC 2 seconds seem like way too long?
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.
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.
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?
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
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?)
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.
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.
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+
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
Closed: 24 years ago
Resolution: --- → FIXED
>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.
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).
*** 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.

Attachment

General

Creator:
Created:
Updated:
Size: