Skip to content

Commit

Permalink
Fix perf regression in LayoutBoxModelObject::RelativePositionOffset()
Browse files Browse the repository at this point in the history
In r597543 we introduced a performance regression
due to the changes in LayoutBoxModelObject::RelativePositionOffset().
One of the main differences is that AvailableHeight|Width()
was called always as part of the method,
while that was not the case before.

This patches moves the calls to AvailableHeight|Width()
to the moment where they are needed, trying to minimize
the impact in performance and come back to previous numbers.

No new tests as it's already covered by existent ones.

BUG=893884,835607

Change-Id: Id8aaba4736a821af9f401492206840c12a2be0e6
Reviewed-on: https://chromium-review.googlesource.com/c/1273117
Commit-Queue: Manuel Rego <rego@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Cr-Commit-Position: refs/heads/master@{#599034}
  • Loading branch information
mrego authored and Commit Bot committed Oct 12, 2018
1 parent aefd059 commit 4226ddf
Showing 1 changed file with 25 additions and 16 deletions.
41 changes: 25 additions & 16 deletions third_party/blink/renderer/core/layout/layout_box_model_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -732,19 +732,15 @@ LayoutSize LayoutBoxModelObject::RelativePositionOffset() const {
// https://drafts.csswg.org/css-grid/#grid-item-sizing
base::Optional<LayoutUnit> left;
base::Optional<LayoutUnit> right;
LayoutUnit available_width = containing_block->AvailableWidth();
LayoutUnit available_height = containing_block->AvailableHeight();
bool has_override_containing_block_content = false;
if (HasOverrideContainingBlockContentWidth()) {
DCHECK(HasOverrideContainingBlockContentHeight());
has_override_containing_block_content = true;
available_width = OverrideContainingBlockContentWidth();
available_height = OverrideContainingBlockContentHeight();
if (!StyleRef().Left().IsAuto() || !StyleRef().Right().IsAuto()) {
LayoutUnit available_width = HasOverrideContainingBlockContentWidth()
? OverrideContainingBlockContentWidth()
: containing_block->AvailableWidth();
if (!StyleRef().Left().IsAuto())
left = ValueForLength(StyleRef().Left(), available_width);
if (!StyleRef().Right().IsAuto())
right = ValueForLength(StyleRef().Right(), available_width);
}
if (!StyleRef().Left().IsAuto())
left = ValueForLength(StyleRef().Left(), available_width);
if (!StyleRef().Right().IsAuto())
right = ValueForLength(StyleRef().Right(), available_width);
if (!left && !right) {
left = LayoutUnit();
right = LayoutUnit();
Expand Down Expand Up @@ -784,19 +780,32 @@ LayoutSize LayoutBoxModelObject::RelativePositionOffset() const {

base::Optional<LayoutUnit> top;
base::Optional<LayoutUnit> bottom;
bool has_override_containing_block_content_height =
HasOverrideContainingBlockContentHeight();
if (!StyleRef().Top().IsAuto() &&
(!containing_block->HasAutoHeightOrContainingBlockWithAutoHeight() ||
!StyleRef().Top().IsPercentOrCalc() ||
containing_block->StretchesToViewport() ||
has_override_containing_block_content)) {
top = ValueForLength(StyleRef().Top(), available_height);
has_override_containing_block_content_height)) {
// TODO(rego): The computation of the available height is repeated later for
// "bottom". We could refactor this and move it to some common code for both
// ifs, however moving it outside of the ifs is not possible as it'd cause
// performance regressions (see crbug.com/893884).
top = ValueForLength(StyleRef().Top(),
has_override_containing_block_content_height
? OverrideContainingBlockContentHeight()
: containing_block->AvailableHeight());
}
if (!StyleRef().Bottom().IsAuto() &&
(!containing_block->HasAutoHeightOrContainingBlockWithAutoHeight() ||
!StyleRef().Bottom().IsPercentOrCalc() ||
containing_block->StretchesToViewport() ||
has_override_containing_block_content)) {
bottom = ValueForLength(StyleRef().Bottom(), available_height);
has_override_containing_block_content_height)) {
// TODO(rego): Check comment above for "top", it applies here too.
bottom = ValueForLength(StyleRef().Bottom(),
has_override_containing_block_content_height
? OverrideContainingBlockContentHeight()
: containing_block->AvailableHeight());
}
if (!top && !bottom) {
top = LayoutUnit();
Expand Down

0 comments on commit 4226ddf

Please sign in to comment.