Open
Bug 719177
Opened 13 years ago
Updated 2 years ago
Use UpdateOverflow hint more
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
REOPENED
mozilla12
People
(Reporter: MatsPalmgren_bugz, Unassigned)
References
Details
(Keywords: perf)
Attachments
(6 files, 3 obsolete files)
5.34 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
3.88 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
10.37 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
18.15 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
3.49 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
Follow-up from bug 524925 which fixed outline and transform properties
to use UpdateOverflow.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #107)
> I also suggest filing a followup bug to have a good look at CalcDifference
> implementations and see which reflow hints can be converted to
> UpdateOverflow. I think there's quite a few that can be converted...
Mats, can you do this?
This should be some cheap and easy performance wins...
Assignee: nobody → matspal
Reporter | ||
Comment 2•13 years ago
|
||
Attachment #590378 -
Flags: review?(roc)
Reporter | ||
Comment 3•13 years ago
|
||
Attachment #590379 -
Flags: review?(roc)
Reporter | ||
Comment 4•13 years ago
|
||
Attachment #590380 -
Flags: review?(roc)
Reporter | ||
Comment 5•13 years ago
|
||
Attachment #590381 -
Flags: review?(roc)
Reporter | ||
Comment 6•13 years ago
|
||
Attachment #590382 -
Flags: review?(roc)
Reporter | ||
Comment 7•13 years ago
|
||
Fwiw, green on Try: https://tbpl.mozilla.org/?tree=Try&rev=5e40e8880f93
Comment on attachment 590378 [details] [diff] [review]
Changes to 'text-shadow' and 'box-shadow' only needs to update the overflow areas and repaint, not reflow. Changes to border style/color/etc only needs repaint, not SyncFrameView.
Review of attachment 590378 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/style/nsStyleStruct.cpp
@@ +2794,5 @@
> if (lhs == rhs)
> return NS_STYLE_HINT_NONE;
>
> + const nsChangeHint kUpdateOverflowAndRepaintHint =
> + NS_CombineHint(nsChangeHint_UpdateOverflow, nsChangeHint_RepaintFrame);
This can be static as well as const
Attachment #590378 -
Flags: review?(roc) → review+
Attachment #590379 -
Flags: review?(roc) → review+
Comment on attachment 590380 [details] [diff] [review]
Changes to '-moz-text-blink' and some 'text-decoration-style' values only need to update the overflow areas and repaint, not reflow.
Review of attachment 590380 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/style/nsStyleStruct.cpp
@@ +2745,5 @@
> + return NS_STYLE_HINT_REFLOW;
> + }
> +
> + const nsChangeHint kUpdateOverflowAndRepaintHint =
> + NS_CombineHint(nsChangeHint_UpdateOverflow, nsChangeHint_RepaintFrame);
can be static
Attachment #590380 -
Flags: review?(roc) → review+
nsTextFrame::SetSelectedRange does some reflowing to update overflow areas in case the selection drawing requires overflow area changes. I think probably nsTextFrame::UnionAdditionalOverflow and SetSelectedRange can be simplified now. Maybe that requires another bug.
Comment on attachment 590381 [details] [diff] [review]
Introduce NS_STYLE_HINT_OVERFLOW as a short-hand for nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_UpdateOverflow).
Review of attachment 590381 [details] [diff] [review]:
-----------------------------------------------------------------
I actually prefer the overflowAndRepaint names. It's just more clear and doesn't impact the wordiness of the code much. So I'd prefer to just leave it as is (and get rid of more of those shortcuts).
::: layout/base/nsChangeHint.h
@@ +143,5 @@
> nsChangeHint_ClearDescendantIntrinsics | \
> nsChangeHint_NeedDirtyReflow)
> #define NS_STYLE_HINT_REFLOW \
> nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_ReflowFrame)
> +#define NS_STYLE_HINT_OVERFLOW \
NS_STYLE_HINT_UPDATE_OVERFLOW I think
Attachment #590381 -
Flags: review?(roc) → review-
Attachment #590382 -
Flags: review?(roc) → review+
I haven't done an audit myself, but one more opportunity I know about is nsStyleSVGReset::CalcDifference ... mFilter can update overflow area instead of reflowing, and mClipPath and mMask don't even need to do that (since they don't affect the overflow area).
Reporter | ||
Comment 13•13 years ago
|
||
I'd like a short-hand for this in some form, let's go with
NS_STYLE_HINT_UPDATE_OVERFLOW for now since it follows the current scheme.
We can re-organize / rename the nsChangeHint stuff in a separate bug.
The change is just s/NS_STYLE_HINT_OVERFLOW/NS_STYLE_HINT_UPDATE_OVERFLOW/
Attachment #590381 -
Attachment is obsolete: true
Attachment #590606 -
Flags: review?(roc)
Reporter | ||
Comment 14•13 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> nsTextFrame::SetSelectedRange does some reflowing to update overflow areas
> in case the selection drawing requires overflow area changes. I think
> probably nsTextFrame::UnionAdditionalOverflow and SetSelectedRange can be
> simplified now. Maybe that requires another bug.
Filed bug 720270.
Attachment #590606 -
Flags: review?(roc) → review+
Reporter | ||
Comment 15•13 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> I haven't done an audit myself, but one more opportunity I know about is
> nsStyleSVGReset::CalcDifference ... mFilter can update overflow area instead
> of reflowing, and mClipPath and mMask don't even need to do that (since they
> don't affect the overflow area).
The SVG effects stuff is really complicated (to me) so I think this deserves
deeper analysis. Filed bug 720272 for that. Let's take the current improvements
as a first step.
OK.
nsStyleDisplay::mClip shouldn't need to reflow. I can't see any more cases (other than the SVG cases).
Reporter | ||
Comment 17•13 years ago
|
||
I also moved the ReconstructFrame cases to the start, and return early if hit.
layout/reftests/bugs/389224-1.html failed with this change, not sure why.
I changed it to trigger the style change of the 'load' event instead of
'MozReftestInvalidate', then it works.
Attachment #590922 -
Flags: review?(roc)
(In reply to Mats Palmgren [:mats] from comment #17)
> layout/reftests/bugs/389224-1.html failed with this change, not sure why.
> I changed it to trigger the style change of the 'load' event instead of
> 'MozReftestInvalidate', then it works.
That sounds like you're just hiding the bug. Please debug!
Reporter | ||
Comment 19•13 years ago
|
||
You're right, sorry. It looked like just flaky test at first but it does indeed
show a real bug with the UpdateOverflow code. Same as bug 720987 if I'm not mistaken.
I'll post the bug fix there and block on that.
Depends on: 720987
Reporter | ||
Comment 20•13 years ago
|
||
Updated to tip since there was a conflicting change to
nsStyleVisibility::CalcDifference
Attachment #590379 -
Attachment is obsolete: true
Attachment #591672 -
Flags: review+
Reporter | ||
Comment 21•13 years ago
|
||
Identical to the last version but without the change to the test so
if you already did review the code part you can just rubber-stamp this.
The fix in bug 720987 makes the test pass. The problem was that the
stored overflow areas is the clipped areas and there's no code to
pick up the full unclipped child overflow again in the current code
so the areas remained the same after the style change.
Try results: https://tbpl.mozilla.org/?tree=Try&rev=4f0758b3d061
Attachment #590922 -
Attachment is obsolete: true
Attachment #590922 -
Flags: review?(roc)
Attachment #591692 -
Flags: review?(roc)
Attachment #591692 -
Flags: review?(roc) → review+
Reporter | ||
Comment 22•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f11aaac24dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f222fbece983
https://hg.mozilla.org/integration/mozilla-inbound/rev/0246973f2513
https://hg.mozilla.org/integration/mozilla-inbound/rev/13739446fc8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/07bd73451aa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cae7e5c62ae
Flags: in-testsuite+
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
Comment 23•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2cae7e5c62ae
https://hg.mozilla.org/mozilla-central/rev/07bd73451aa4
https://hg.mozilla.org/mozilla-central/rev/13739446fc8e
https://hg.mozilla.org/mozilla-central/rev/0246973f2513
https://hg.mozilla.org/mozilla-central/rev/f222fbece983
https://hg.mozilla.org/mozilla-central/rev/8f11aaac24dc
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Reporter | ||
Comment 24•13 years ago
|
||
These patches will be backed out in bug 724432. I intend to re-land most of it
with some additional fixes here, except text-shadow which is handled by bug 723669.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
What I think I'd written elsewhere: if we want to do this, I think we should:
* rename UpdateOverflow to UpdateAncestorOverflow
* add a new hint called UpdateSelfOverflow
* implement the new hint such that we start from accumulating the element's overflow from its descendants and its on characteristics (rather than starting with the change from pre-transform overflow to post-transform overflow on the element)
Also, I'd prefer not to have NS_STYLE_HINT_UPDATE_OVERFLOW for two reasons:
* we should get rid of NS_STYLE_HINT_* and use nsChangeHint_*
* I'd rather actually have to have the combination written out so that it's clearer.
A good bit of the discussion about relanding this is in bug 723669.
Depends on: 984226
The prerequisites for doing this are now happening in bug 984226.
Reporter | ||
Updated•10 years ago
|
Assignee: mats → nobody
Depends on: 1131371
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•