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)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
31.60 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
as described. It's reasonably straightforward. It should reduce Tp a bit.
Attachment #183249 -
Flags: superreview?(dbaron)
Attachment #183249 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•19 years ago
|
||
fixes Camino, revs UUID
Attachment #183249 -
Attachment is obsolete: true
Attachment #183322 -
Flags: superreview?(dbaron)
Attachment #183322 -
Flags: review?(dbaron)
Assignee | ||
Updated•19 years ago
|
Attachment #183249 -
Flags: superreview?(dbaron)
Attachment #183249 -
Flags: review?(dbaron)
Assignee | ||
Updated•19 years ago
|
Attachment #183322 -
Flags: review?(dbaron) → review?(bzbarsky)
Assignee | ||
Comment 3•19 years ago
|
||
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...
Comment 4•19 years ago
|
||
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).
Assignee | ||
Comment 5•19 years ago
|
||
(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 6•19 years ago
|
||
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+
Assignee | ||
Comment 7•19 years ago
|
||
(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.
Assignee | ||
Comment 8•19 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•19 years ago
|
||
This reduced Tp by 15-20ms on btek and luna. Worth having, but probably not worth putting in FF1.5.
Comment 10•19 years ago
|
||
(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...
Comment 11•19 years ago
|
||
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.
Comment 13•19 years ago
|
||
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...
Assignee | ||
Comment 14•19 years ago
|
||
hmm ... it would be interesting to know what the gain is from the history, then
Comment 15•19 years ago
|
||
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 18•19 years ago
|
||
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.
Comment 19•19 years ago
|
||
> 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...
Comment 20•19 years ago
|
||
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...)
Assignee | ||
Comment 21•19 years ago
|
||
(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 22•19 years ago
|
||
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.
Description
•