Closed Bug 293714 Opened 19 years ago Closed 19 years ago

Be smart about guessing when a vertical scrollbar is needed

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

A long time ago I read something in Hyatt's blog about how in Safari they do
something clever to guess/remember whether a page needs a vertical scrollbar or
not. (The reason being, of course, that if you guess incorrectly then you need
to reflow it an extra time during load when you discover that your assumption
was incorrect and you need to add or remove the vertical scrollbar.)

Now that HTML scrollframes are sane, I've cooked up a patch to do this for us.
The algorithm is pretty clear in the patch. Basically we store a flag in the
global history that says whether the page had a vertical scrollbar or not. If
that hint is not present for some reason then we assume the page needs a
vertical scrollbar.
Attached patch fix (obsolete) — Splinter Review
as described. It's reasonably straightforward. It should reduce Tp a bit.
Attachment #183249 - Flags: superreview?(dbaron)
Attachment #183249 - Flags: review?(dbaron)
Attached patch fix #2Splinter Review
fixes Camino, revs UUID
Attachment #183249 - Attachment is obsolete: true
Attachment #183322 - Flags: superreview?(dbaron)
Attachment #183322 - Flags: review?(dbaron)
Attachment #183249 - Flags: superreview?(dbaron)
Attachment #183249 - Flags: review?(dbaron)
Keywords: perf
Attachment #183322 - Flags: review?(dbaron) → review?(bzbarsky)
I thought about trying to change this so it doesn't force embedders to change,
but I don't see how without introducing a new interface. Adding these methods to
nsIBrowserHistory is not a good idea because currently Gecko core doesn't depend
on nsIBrowserHistory and in fact Firefox and Seamonkey have different
nsIBrowserHistories...
Would it make sense to have history QI to nsIProperties or some such instead? 
That would allow storage of richer stuff than flags (if we need to!) and allow
no interface changes.

That said, I think the proposed nsIGlobalHistory2 API is ok, especially if we
document that the exact value of the flags should be opaque to the history impl
(that is, the impl should just store the integer, not attempt to guess what the
integer means).
(In reply to comment #4)
> Would it make sense to have history QI to nsIProperties or some such instead? 
> That would allow storage of richer stuff than flags (if we need to!) and allow
> no interface changes.

Yes, but it would be considerably harder to implement because we then require
history implementors to support an arbitrary number of per-URL properties (if
they want to support this).

> That said, I think the proposed nsIGlobalHistory2 API is ok, especially if we
> document that the exact value of the flags should be opaque to the history
> impl (that is, the impl should just store the integer, not attempt to guess
> what the integer means).

That's certainly the intent.
Comment on attachment 183322 [details] [diff] [review]
fix #2

>Index: docshell/base/nsIGlobalHistory2.idl

Please put the new stuff at the end of the interface?

>Index: layout/generic/nsGfxScrollFrame.cpp
> nsHTMLScrollFrame::nsHTMLScrollFrame(nsIPresShell* aShell, PRBool 
>+    mHadNonInitialReflow(PR_FALSE)

Couldn't we store that boolean in mInner and save some space (since the inner
can have 6 more bits before it actually gets bigger)?  Is there a reason to
store this on the outer?

>+nsGfxScrollFrameInner::GetVScrollbarHintFromGlobalHistory(PRBool* aVScrollbarNeeded)

Can this method assert up front that !mDidLoadHistoryVScrollbarHint?  If not,
it should probably return mHistoryVScrollbarHint when
mDidLoadHistoryVScrollbarHint is true.	But I think we can add that assertion
here...

r+sr=bzbarsky with that
Attachment #183322 - Flags: superreview?(dbaron)
Attachment #183322 - Flags: superreview+
Attachment #183322 - Flags: review?(bzbarsky)
Attachment #183322 - Flags: review+
(In reply to comment #6)
> Please put the new stuff at the end of the interface?

Will do.

> >Index: layout/generic/nsGfxScrollFrame.cpp
> > nsHTMLScrollFrame::nsHTMLScrollFrame(nsIPresShell* aShell, PRBool 
> >+    mHadNonInitialReflow(PR_FALSE)
> 
> Couldn't we store that boolean in mInner and save some space (since the inner
> can have 6 more bits before it actually gets bigger)?  Is there a reason to
> store this on the outer?

Not really. I'll do that.

> >+nsGfxScrollFrameInner::GetVScrollbarHintFromGlobalHistory(PRBool*
aVScrollbarNeeded)
> 
> Can this method assert up front that !mDidLoadHistoryVScrollbarHint?  If not,
> it should probably return mHistoryVScrollbarHint when
> mDidLoadHistoryVScrollbarHint is true.	But I think we can add that assertion
> here...

Yep. Will do.
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
This reduced Tp by 15-20ms on btek and luna. Worth having, but probably not
worth putting in FF1.5.
(In reply to comment #9)
> This reduced Tp by 15-20ms on btek and luna.
Doesn't Tp run with a clean profile, therefore a clean history? That would
suggest that the Tp gain isn't related to the history portion of the patch...
Tp does run with a clean profile, but it cycles through the Tp pageset 5 times.
 So after the first time through this patch would kick in.
Well, each time the URLs have a different query string, so actually the history
stuff shouldn't help.
Interesting.  So I guess the win here was completely from assuming that a
typical page _will_ need a scrollbar instead of our old assumption that it will
not...
hmm ... it would be interesting to know what the gain is from the history, then
We could try tweaking a tinderbox to not nix the profile as a test...
I don't think that would help, since the previous time is part of the URL.

(But I think this patch would help for any iframes / frames within the pages.)
...assuming that both the frame/iframe and the scrollbar state are stored in
history.
Comment on attachment 183322 [details] [diff] [review]
fix #2

>+  // If we've had at least one non-initial reflow, then just assume
>+  // the state of the vertical scrollbar will be what we determined
>+  // last time.

>-  // Don't assume a vertical scrollbar if we're not allowed to have
>-  // one
>-  PRBool canHaveVerticalScrollbar =
>-    aState->mStyles.mVertical != NS_STYLE_OVERFLOW_HIDDEN;
>-  if (!canHaveVerticalScrollbar)
>-    currentlyUsingVScrollbar = PR_FALSE;
>+  PRBool currentlyUsingVScrollbar = GuessVScrollbarNeeded(*aState);

By my reading of the patch these are the two hunks that should make most of the
difference, that is to say that before the patch went in each reflow started
without a vertical scrollbar and after the patch went in each reflow assumes
the vertical scrollbar state won't change, which means only misguessing the
scrollbar once on large pages.
> which means only misguessing the scrollbar once on large pages.

And we guess an initial state of "need scrollbar" if given no other info, so on
a large page we'll never misguess...
Ah, so on the first reflow you guess no scrollbar, then on the second reflow you
check history and failing that guess scrollbar, then on subsequent reflows you
guess same as last time? (Although that wouldn't appear to work in the case of a
long page that appears slowly, e.g. an LXR search...)
(In reply to comment #18)
> By my reading of the patch these are the two hunks that should make most of the
> difference, that is to say that before the patch went in each reflow started
> without a vertical scrollbar 

No. Before the patch, every reflow assumed the vertical scrollbar state would be
the same as before; the first reflow would assume no vertical scrollbar.

> (Although that wouldn't appear to work in the case of a
> long page that appears slowly, e.g. an LXR search...)

If the slowness is consistent, it should work pretty well for that case.

The history is only going to help on top-level pages that don't need a vertical
scrollbar. I'm not sure how many of our Tp pages actually fall into that
category. It probably doesn't really matter what the true numbers are unless
someone wants this to go in 1.8.
Comment on attachment 183322 [details] [diff] [review]
fix #2

>-  PRBool currentlyUsingVScrollbar = mInner.mHasVerticalScrollbar;
I'm sorry, I missed this bit. That explains everything, thanks.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: