Closed Bug 73382 Opened 23 years ago Closed 23 years ago

Cleanup view manager code

Categories

(Core :: Web Painting, defect, P3)

Product:
Core
Component:
Web Painting
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.0

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(2 files, 9 obsolete files)

View interfaces cleanup
38.81 KB, patch
Details | Diff | Splinter Review
Fix bug in nsViewManager::SetViewVisibility
142.89 KB, 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.)
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.




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.
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.
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

Creator:
Created:
Updated:
Size: