Closed Bug 73382 Opened 24 years ago Closed 23 years ago

Cleanup view manager code

Categories

(Core :: Web Painting, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(2 files, 9 obsolete files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
kmcclusk
: review+
attinasi
: superreview+
Details | Diff | Splinter Review
The view manager code needs to be cleaned up. Here are the work items: -- Documentation of the z-index algorithm -- Convert tabs to spaces -- Remove dead code -- Debug code to dump display list None of these changes should affect any runtime behavior.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.0
The above patch converts tabs to spaces, makes indenting consistent, and adds some comments. It does not change any actual code. This should be a no-brainer and I'd like to get it in as soon as possible as the first step. kevin, marc, could you r/sr this?
Comment on attachment 52850 [details] [diff] [review] Items 1 and 2; comments and whitespace changes ONLY r=kmcclusk@netscape.com.
Attachment #52850 - Flags: review+
Comment on attachment 52850 [details] [diff] [review] Items 1 and 2; comments and whitespace changes ONLY sr=attinasi (rubber-stamping - I assume you verified that no *code* actually changed using something more reliable then my eyes, like object file diffs).
Attachment #52850 - Flags: superreview+
Checked in. Now on to the next step...
(BTW I verified the change using "diff -bu" to ignore whitespace changes and eyeballing the results ...)
I could use r/sr from you guys for patch 53049. Thanks!
Comment on attachment 53049 [details] [diff] [review] Remove nsViewManager2 from build r=kmcclusk@netscape.com
Attachment #53049 - Flags: review+
Comment on attachment 53049 [details] [diff] [review] Remove nsViewManager2 from build Sorry, missed a file. I'll attach another patch.
Attachment #53049 - Attachment is obsolete: true
Sorry about that... Why are you up so late anyway? :-) How do I remove nsViewManager2.cpp for the Mac? camelot.mozilla.org doesn't seem to be working.
Attachment #52850 - Attachment is obsolete: true
Comment on attachment 53068 [details] [diff] [review] Clean up code in nsViewManager.cpp This is a separate patch which needs separate review. It removes some "#if 0" code and some unused variables, and fixes gcc warnings about printf paramers. The main thing it does is to get rid of the duplicate Refresh(nsRect) code. We push everything through the generic Refresh(nsIRegion) path. Because regions are always in device coordinates we have to move a bit of the logic around, but otherwise nothing much has changed.
Oops, that didn't work too well. http://bugzilla.mozilla.org/attachment.cgi?id=53068 This is a separate patch which needs separate review. It removes some "#if 0" code and some unused variables, and fixes gcc warnings about printf paramers. The main thing it does is to get rid of the duplication of Refresh() code. We remove Refresh(nsRect) and push everything through the generic Refresh(nsIRegion) path. Because regions are always in device coordinates we have to move a bit of the logic around, but otherwise nothing much has changed. Eventually I'd like to push region information from the platform paint event right into RenderViews so we can optimize the display list more. Apart from getting rid of redundant code, this cleanup is a step in that direction.
Blocks: 92580
Comment on attachment 53068 [details] [diff] [review] Clean up code in nsViewManager.cpp Nice cleanup Robert! r=kmcclusk@netscape.com
Attachment #53068 - Flags: review+
Comment on attachment 53068 [details] [diff] [review] Clean up code in nsViewManager.cpp rs=attinasi
Attachment #53068 - Flags: superreview+
Comment on attachment 53060 [details] [diff] [review] Added makefile.win super-luvin' by attinasi
Attachment #53060 - Flags: superreview+
That last checkin caused bug 106355 on Linux
OK, those patches are checked in and messes cleaned up. Thanks. For my next trick, I want to fix the view API. There are a bunch of methods in nsIView that only the view manager should be able to call (just about all the state changing methods). Most changes to view state currently go through the view manager and we should enforce that where appropriate. (Otherwise it's going to be a pain to audit all the callers to stuff like SetBounds when we change its semantics.) So I propose removing the private methods from nsIView, and expose the private methods through nsView* instead. I'll fix things up so that all nsIView implementations inherit from nsView. Then inside the view module we can pass around nsView* pointers instead of nsIView*, using NS_STATIC_CAST to obtain an nsView* from an nsIView* at public view manager interfaces. I will post a patch to do this shortly. Let me know if you think I'm barking up the wrong tree.
Attachment #53060 - Attachment is obsolete: true
Attachment #53068 - Attachment is obsolete: true
This patch does what I just said. It's big, but it doesn't change any logic. Its main purpose is to let view code refer to all views through nsView* and refer to all view managers through nsViewManager*. This sets us up to be able to move methods from the public nsIView/nsIViewManager classes down to nsView/nsViewManager if they need not or should not be exposed outside the view code. It should also be a slight performance win and code elegance win. I want the public view interfaces narrowed so that fixing event handling and view bounds "above and to the left" handling can be done in a self-contained way. That patch leaves most of the "privatization" for future work, but it does "privatize" some of the structural access and modification methods while changing them to use nsView*, to reduce the number of casts between nsView* and nsIView*. The patch also makes nsZPlaceholderView a child of nsView and moves it to nsViewManager.h so nsView can see it. Incidentally, the patch also removes a few obsolete methods that weren't used by anyone.
The patch looks good, but the interface headers are missing. I like the idea, it removes a bunch of unnecessary virtual calls in addition to encapsulating the view module better. Can you add the difs for the nsIView and nsIViewManager files too?
Comment on attachment 55502 [details] [diff] [review] Patch to convert nsIView* references to nsView* within view module r=kmcclusk@netscape.com. Nice cleanup!
Attachment #55502 - Flags: review+
Attachment #55502 - Attachment is obsolete: true
Comment on attachment 56599 [details] [diff] [review] Patch including changes to nsIView/nsIViewManager r=kmcclusk@netscape.com
Attachment #56599 - Flags: review+
Good catch Marc ... sorry about that. The last attachment is the same as patch 55502 but includes the changes to nsIView/nsIViewManager ... just hiding several methods that should not be used outside the view module. After this patch is in, we can shrink the public interfaces some more.
Comment on attachment 56599 [details] [diff] [review] Patch including changes to nsIView/nsIViewManager sr=attinasi
Attachment #56599 - Flags: superreview+
Attachment #56599 - Attachment is obsolete: true
Checked in. Thanks guys! Now for my next trick...
Looks like this caused a crash on http://www.dhtmlnirvana.com, bug 108651. Is there a quick fix, or should we back this out?
It was backed out, and rightly so. The fix should be simple but I will need to properly test it. Not doing so was inexcusable.
(FTR, that regression was fixed and everything is good again.)
Attached patch View interfaces cleanup (deleted) — Splinter Review
OK, here's this week's project :-). I've attached a patch which represents the changes I believe will totally clean up the public view interfaces and get them into an adequate state for Mozilla 1.0 and hopefully beyond. The main thrusts are 1) remove methods that should be private to the view code 2) Garbage collect unused/incoherent methods 3) Move view mutation methods to the view manager 4) Replace horrible interface code (e.g., explicit access to flags variable) with cleaner API calls 5) Extend and tweak interfaces to fix the serious design flaws (e.g., problems with views that extend above and to the left of their frame's origin). The attachment here is not a complete patch, but rather an annotated diff of just the public interface changes, for easy review. I have a complete patch that fixes up all the clients, view code, etc, in my tree. I've even tested it :-). I'll attach the full patch when/if these ideas pass muster.
BTW, after this part is done, here are my expected work items: DeCOMify newly private methods Move event handling into nsViewManager Make event handling use CreateDisplayList (to fix z-index and events) Audit users of nsIView::GetPosition and nsIView::GetBounds, then fix nsContainerFrame::SyncFrameViewAfterReflow to size views to contain left-or-above content Remove nsIClipView stuff from the view code and just use the CLIPCHILDREN flag (to unify the clipping architecture; currently I suspect we are losing badly because there are two ways for views to clip and often the code only handles one of them) Put in support for hierarchy of viewmanagers (handle nsViewManager::SetRootView case where aWidget == null and aView has a non-null parent with a different view manager) (i.e., support transparent IFRAMEs, overlapping IFRAMEs with other transparent content, etc) Fix opacity model to conform to SVG (requires backbuffer stack) Optimize view storage (we should be able to cut view memory usage in half, easily)
Robert: I reviewed attachment 57738 [details] [diff] [review] and it looks like your on the right track. Removing all of the cruft from years gone by is great. Other aspects of the patch also look sound.
When you get to this item: "Fix opacity model to conform to SVG (requires backbuffer stack" lets have another discussion. At some point we need to push the compositing code into gfx module and create a interface which would allow us to take advantage of hardware acceleration when possible. I think the bigger issue for the viewmanager in support of SVG will be comming up with a more sophisticated rendering-pipeline which allows us to do compositing in conjuction with other filter effects. It would also be desirable if the rendering pipeline where "pluggable" so 3rd party vendors could add custom filter effects etc.
btw, I think any SVG specific support in the ViewManager should be post Mozilla 1.0.
Since you're already cleaning up this code would you mind also getting rid of the spaces around '::' in class methods, i.e. replace: nsViewManager2 :: SetRootView() with: nsViewManager2::SetRootView() to make this code match what 99% of the rest of mozilla code looks like. Every single time I end up in view code and I try to search for a ::Paint() method or something I end up not finding it until I realize that here I need to look for :: Paint. Maybe it's just me, and I don't wanto step on anyones toes wrt coding style, but I see this style doing us nothing but harm.
"getting rid of the spaces around '::' in class methods" Good idea. The person who introduced this is long gone, so I don't think were stepping on anyone's toes.
Attached patch Fix up view interfaces as previously described (obsolete) (deleted) — Splinter Review
Here it is, as promised. Check it out. The sooner this gets in, the sooner I can move along my TODO list :-). (This includes the " :: " -> "::" changes by the way.) Note that in many places I say "XXX use document order when we know how to do that". Currently layout doesn't really know where it is in document order when we're inserting a view, although we're usually inserting something at the end of the document. If we want to support CSS2 z-index to the letter, then we'll eventually have to have layout not lie to the view system in those rare cases where view insertion order does not correspond to document order.
I agree that SVG architecture work is post-1.0. However, just fixing -moz-opacity doesn't require major architecture work IMHO.
Comment on attachment 58333 [details] [diff] [review] Fix up view interfaces as previously described r=kmcclusk@netscape.com
Attachment #58333 - Flags: review+
Comment on attachment 58333 [details] [diff] [review] Fix up view interfaces as previously described sr=attinasi
Attachment #58333 - Flags: superreview+
I see people mentioning SVG. The SVG branch which will be landing RSN uses libart to do the drawing, rendering to a buffer which then bitblts to the screen. Of course, that branch currently doesn't handle group opacity yet, so...
So, looks like this clean up landed around 10pm today, actually caused a slow down on page-load about 5% (loss of ~60ms), see btek tinderbox number. Can someone take a look?
dbaron and I had already started looking, although it's a big patch :-] Of note is that the biggest gainer was a copy of an lxr.mozilla.org page (for nsVoidBTree.cpp) up +18%. The rest of the pages were between +3 and +9%, with two exceptions: an iplanet page with a large background image down 6%, and an old version of www.microsoft.com up 14%. I'm building on windows now to confirm this. roc, if you need any testing done, just let me know.
Should it be backed out or is the fix imminent?
This patch wasn't supposed to change any behavior, so nothing obvious springs to mind as to what the problem is. Maybe I just introduced a bug. Another possibility is that we're doing extra invalidations because in some places where we called nsIView::SetVisibility (which doesn't invalidate) we now call nsIViewManager::SetViewVisibility (which does) (ditto for some other methods) ... but then the old code was "wrong". Is there a way for me to run my own page-load tests? I've poked around mozilla.org before but didn't find out how. If I had the tests I could probably narrow down the problem to a small part of the patch and back that part out tonight or tomorrow. OTOH I don't mind the whole patch being backed out right now, but I can't do it myself now ... gotta go to some Thanksgiving stuff :-).
Quick profiles of before/after on the lxr page show that the amount of time spent within nsView::Paint went up by a factor of 3 (from 280/9515 to 834/10195).
BTW, the SetViewVisibility change you mention could have broken the fact that we don't paint the old page when we start paint supression.
Actually, that might be wrong. Even more interesting is the change in children of PresShell::Paint. Before, most of the calls went through to the CanvasFrame. Now they seem to be going much more to other things that have views: before: 160 CanvasFrame::Paint 88 nsContainerFrame::Paint 30 nsSliderFrame::Paint 1 nsBoxFrame::Paint 1 SetClipRect after: 599 nsContainerFrame::Paint 187 CanvasFrame::Paint 21 nsBlockFrame::Paint 18 nsSliderFrame::Paint 4 nsBoxFrame::Paint 2 nsImageBoxFrame::Paint 2 nsHTMLFrameOuterFrame::Paint
It's also interesting that the time spent within nsRootBoxFrame::Paint (including its children) went up from 87 hits to 593 hits. That suggests that almost all of the new painting time is spent painting the *chrome*.
I'm beginning to wonder how skewed these numbers are by jprof's 100-stack-frame limit (which could easily be raised). Anyway, I'll attach the before and after profiles, in a .tar.gz.
Attached file roc-regression-profiles.tar.gz (obsolete) (deleted) —
The numbers look essentially the same on win32, although there are no "standout" pages as on Linux. The script that runs this (apache/linux based) is at http://lxr.mozilla.org/mozilla/source/tools/page-loader/
I'm able to reproduce the slowdown in an optimized build. I tried disabling the obvious parts of the patch (reverting to nsIView::SetVisibility and nsIView::SetContentTransparency) but that had no effect. dbaron's jprofs (thanks!!) show that clearly there is a lot more painting going on. They also seem to show a resize reflow in the after case that isn't there in the before case, but I'm not sure if that's significant. Anyway I still haven't got much of a clue about what the problem is here. I would like to back out the patch. Do I need approval for that, and if so, could someone give it to me? (goodnight!)
backout in light of perf regression doesn't need review unless you want more eyes on the reverse-patch for the changes that went in with proper review. Have there been other changes to any of the files since your checkin? /be
Another data point -- in a build with --disable-double-buffer, it's clear that every invalidate is resulting in a full-window repaint. When I type in this textarea in this bug, it's clear that the entire window repaints, whereas it used to be that only a part of the textarea itself repointed.
I backed it out just now.
Page load numbers are back down. Now I just have to figure out why they went up in the first place...
dont know about you guys, maybe its just the net is faster during this holiday and just happens to be the reason I get faster page loading lately (i'm on dialup).. but page loading seems pretty snappy with the last several nightlies. I guess I'd have to check builds after 11-24 for a few days with the builds from 11-23 and earlier to see if that is still the case. Or maybe its just the nsURLparser.cpp file that is loading page content faster, that I'm seeing. p3-733, 512MB.
bbaetz: I'm not sure how you guys are handling foreignObject right now, but you will surely need help from the view system there eventually. We can address this post-1.0.
roc: Currently (as in this instant) it only works on win32, but there are patches to get it working on unix/mac, too. It works fine, but I suspect that we will need view system help to get opacity + <foreignobject> working correctly, although I'm not sure.
Attached patch Fixed performance problems (I think) (obsolete) (deleted) — Splinter Review
I believe I have found and fixed the problem --- setting the clip on a view was forcing it to be repainted, and this led to views being repainted after every reflow, more or less. I think that some invalidation *should* be done if the clip rect actually changed, but for now I'll mimic the old behavior and not do any. John, please try this patch to verify it does not whack page load times.
Attachment #58333 - Attachment is obsolete: true
Attachment #58894 - Attachment is obsolete: true
Gee, roc. Sorry to say, but this new patch also whacks page load times. ;-] The trunk with patch 59166 is about 1.5% _faster_ on win2k, and about 2.5% faster on Linux, compared with the current trunk, for cached page loads (probably about the same on uncached loads, but I need more samples to say for sure). What's up with that? Do these results make sense, or did I make a mistake (I don't think so; not on two different builds).
Woooo. Honestly, I don't know why this would happen. One possible reason is that I do some extra checking so that if you try to change the state of a view that is not actually in the view hierarchy yet, we don't invalidate it. That might be doing something. Another obvious possibility is that the patch has some subtle bug that doesn't repaint or otherwise do enough work. But I haven't seen anything in testing (obviously, or I would have fixed it). How about we check this in, and I get 2.5% in my "page load time account" to use up later when I fix some more bugs? :-)
If we'd do that we'd need to set a timeline for you to fix the problems, like maybe a week. I can't say if this is acceptable in general or not tho, mozilla.org needs to make that decision.
I think you misunderstand me. I don't know of anything wrong with this patch (well, one thing, which I'll get to in my next comment). What I meant is that there are other bugs in my bug list which I suspect may be hard to fix without impacting page load time.
While studying the previous patch for possible inadvertent performance improvements, I found a bug in SetViewVisibility --- failed to actually change the visibility if the view was not yet inserted into the view hierarchy (which I don't *think* is ever the case here, but you never know). John, sorry about this, but if you don't mind rerunning the page load tests again...
Attachment #59166 - Attachment is obsolete: true
Well, cleanup that results in pageload time regressions on the order of several % does not seem acceptable to me. Or is this more than just cleanup?
Oh, eh ignore me, I don't know what I'm talking about.
(Sorry, if my flip comment about 'whacks page load times' was misinterpreted :-]) roc: the differences between attachment 59166 [details] [diff] [review] and attachment 59293 [details] [diff] [review] are all in nsViewManager.cpp, right? In other words, for my already patched build, I only need to revert nsViewManager.cpp to the trunk rev. and then apply the part of the patch for that file and rebuild. Yes?
with the new rev for nsViewManager.cpp applied along with the previous changes, I'm still at 1.0% faster on win32, and still at 2.5% faster on Linux (I should also note that I said 1.5% on win32 earlier, but meant 1.3%). So, basically the same as before (yes, a bit lower on win32, but changes in the .LT. 1% range aren't significant).
I did a quick look through the top 100 URLs list ... didn't notice any problems. Previous problem sites like dhtmlnirvana.com and w3.org/CSS look fine. Browsing and mail seems to work fine. We had it in the tree for a couple of days and apart from the slowdown, no bugs came my way (was over the holidays though). I say we check it in :-).
Assuming everyone's OK with this being checked in, can I please have r and sr reapplied to the new patch so I can actually check it in? Thanks!
Comment on attachment 59293 [details] [diff] [review] Fix bug in nsViewManager::SetViewVisibility r=kmcclusk@netscape.com
Attachment #59293 - Flags: review+
Comment on attachment 59293 [details] [diff] [review] Fix bug in nsViewManager::SetViewVisibility rs=attinasi
Attachment #59293 - Flags: superreview+
I checked it in ... didn't seem to affect page load times at all on Tinderbox
roc, is there a bug on doing invalidation when clip changes? Right now the style system does a reflow on dynamic clip property changes to force things to repaint correctly....
No bug that I know of. File a bug against me and I'll take care of it.
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 (you can query for this string to delete spam or retrieve the list of bugs I've moved)
Target Milestone: mozilla1.0 → mozilla1.0.1
don't move bugs that are in the 1.0 dependency tree. sorry.
Target Milestone: mozilla1.0.1 → mozilla1.0
Sorry for butting in here but I had a performance question. Sometime towards the end of last week, the outliner widget went "nasty" on us. Outliner widgets are now getting invalidated any time focus leaves and then comes back to the window the widget is in. Could this view manager landing last week have caused these invalidations on focus change?
I guess it's possible. For example, we changed calls to nsIView::SetVisibility, which does not invalidate the view, to nsIViewManager::SetViewVisibility, which does invalidate the view. However I don't see which of the changes in layout/xul could have caused problems with the outliner. OTOH I don't know anything about the outliner.
Keywords: mozilla1.0+
I'm closing this. Future work on the view manager should be filed under new bugs. There is no way to verify this; the code was checked in and that's that.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: