More style flushes in FF62 when setting width to an unchanged value

RESOLVED FIXED in Firefox 63

Status

()

defect
P2
normal
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: bholley, Assigned: xidorn)

Tracking

({perf, regression})

62 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62+ unaffected, firefox63+ fixed)

Details

Attachments

(6 attachments)

This is a regression in Firefox 62.

In bug 1471099, we have a testcase where a scroll event listener loops over all ~20 images in the document. For each one it queries offsetTop, and then sets the width. The width is computed to widen the image as it scrolls into view, and images outside of the viewport get their width set to undefined.

There were reports that the testcase in that bug had jankier scrolling in Firefox than in Chrome (mobile only). I had trouble judging the difference, so I modified the testcase to repeat the loop 100 times on every scroll event. With that change, the difference became much more apparent. I'll attach the testcase.

Emilio then said our behavior here changed in 61, because of [1], which is presumably bug 1415330. That landed in 61, but was backed out in bug 1461285.

Here's a profile in Firefox 61 (release): https://perfht.ml/2MJyt2A
And here's a profile in Firefox 62 (beta): https://perfht.ml/2KGd39j

Note that the showFullImg calls take about twice as long. I'd originally thought that we must be flushing on every width set, but then it would take ~20x the time. So I think there may be something going on here with sets from 0px to negative percentages, which is what I see in the logs. I'm out of time to investigate for today.

[1] https://github.com/w3c/csswg-drafts/issues/1898
Posted file oneplus.zip
Testcase. This is the site from bug 1471099, with a modification to make the showFullImg() call in the scroll listener run 100 times. Scrolling down the page leads to jankier image resizing.
Flags: needinfo?(emilio)
I stuck in some performance.now() console logging, and it looks like pre-regression Firefox is slightly better than Chrome (~115ms vs ~140ms). However, post-regression Firefox is in the ~250ms range.
Version: unspecified → 62 Branch
Keywords: perf
P2 since it's a regression.  Emilio -- Do you have the bandwidth to take this?
Assignee: nobody → emilio
Priority: -- → P2
If bug 1461285 can get uplifted to beta, this regression would become nightly only for a while. We may still want to fix it though, for shipping that behavior change. I can try taking it I guess. emilio is traveling back to German and would need more rest I think.
Assignee: emilio → xidorn+moz
Yes, Xidorn has taken care of that bug.

I think we should cycle back with the CSSWG wrt that spec change, since it's a bit annoying and I don't think Blink and WebKit would ever take the inevitable perf regressions it'd cause.
Flags: needinfo?(emilio)
Have you done some performance testing to see how this compares to what we currently do with and without the always_append pref?

It's definitely slower but...
So, I use bholley's testcase with performance.now() timing to print time in console.

* try build with this version: 131±7ms
* m-c build on the base of the try build:
  * append mode: 236±5ms
  * update mode: 131±8ms

I also checked Chrome:
* stable 67: 193±13ms
* canary 69: 1318±35ms

So this would bring back our previous performance with the desired behavior regarding logical properties.

(The performance on Chrome canary is surprising bad... I'm wondering whether we should file a bug to them... It's 10x worse...)
Comment on attachment 8992307 [details]
Bug 1473180 part 1 - Introduce a concept of logical group.

https://reviewboard.mozilla.org/r/257174/#review264332

::: servo/components/style/properties/data.py:197
(Diff revision 1)
>          self.need_index = need_index
>          self.gecko_ffi_name = gecko_ffi_name or "m" + self.camel_case
>          self.cast_type = cast_type
>          self.logical = arg_to_bool(logical)
> +        self.logical_group = logical_group
> +        if self.logical and not logical_group:

nit: maybe:

```
if self.logical:
    assert self.logical_group, "..."
```

Though no strong preference either way.

::: servo/components/style/properties/properties.mako.rs:1029
(Diff revision 1)
>                      </%def>
>                      </%helpers:logical_setter_helper>
>                  }
>              % endif
>              % endfor
>              _ => *self

May be worth to:

```
debug_assert_eq!(self.logical_group(), physical.logical_group())
```

or something like that.

::: servo/components/style/properties/properties.mako.rs:1034
(Diff revision 1)
>              _ => *self
>          }
>      }
>  
> +    /// Return the logical group of this longhand property.
> +    pub fn logical_group(&self) -> Option<LogicalGroup> {

nit: May be worth to make this a jump-table explicitly, it's a fair amount of properties.
Attachment #8992307 - Flags: review?(emilio) → review+
Comment on attachment 8992308 [details]
Bug 1473180 part 2 - Add new algorithm for setting property to be used in later commit.

https://reviewboard.mozilla.org/r/257176/#review264334

Looks good over all, thanks a lot for working on this.

I think I could take another look when the comments are addressed, though most of them are trivial so I'll flag r+.

::: servo/components/style/properties/declaration_block.rs:426
(Diff revision 1)
>      /// Returns whether the property is definitely new for this declaration
>      /// block. It returns true when the declaration is a non-custom longhand
>      /// and it doesn't exist in the block, and returns false otherwise.
>      #[inline]
>      fn is_definitely_new(&self, decl: &PropertyDeclaration) -> bool {
> -        match decl.id() {
> +        decl.id().longhand().map_or(false, |id| !self.longhands.contains(id))

I'd rename `longhand` to `as_longhand` for consistency with `PropertyId::as_shorthand`.

::: servo/components/style/properties/declaration_block.rs:581
(Diff revision 1)
>          true
>      }
>  
> +    /// Prepares updating this declaration block with the given
> +    /// `SourcePropertyDeclaration` and importance, and returns whether
> +    /// there are something to update.

nit: Whether there is something to update.

::: servo/components/style/properties/declaration_block.rs:585
(Diff revision 1)
> +    /// `SourcePropertyDeclaration` and importance, and returns whether
> +    /// there are something to update.
> +    ///
> +    /// # Implementation details
> +    ///
> +    /// When all shorthand is used, it just scan the declaration block

nit: When the `all` shorthand is used.

::: servo/components/style/properties/declaration_block.rs:586
(Diff revision 1)
> +    /// there are something to update.
> +    ///
> +    /// # Implementation details
> +    ///
> +    /// When all shorthand is used, it just scan the declaration block
> +    /// for whether there is anything to update, and return accordingly.

`whether there's anything to update` sounds not very clear. Maybe 'Whether any lognhand value would change'?

::: servo/components/style/properties/declaration_block.rs:589
(Diff revision 1)
> +    ///
> +    /// When all shorthand is used, it just scan the declaration block
> +    /// for whether there is anything to update, and return accordingly.
> +    ///
> +    /// Otherwise, for each new declaration, it scans the declaration
> +    /// block, and record an action for `update` to take. See document

nit: records. Also, 'See the documentation' perhaps?

::: servo/components/style/properties/declaration_block.rs:590
(Diff revision 1)
> +    /// When all shorthand is used, it just scan the declaration block
> +    /// for whether there is anything to update, and return accordingly.
> +    ///
> +    /// Otherwise, for each new declaration, it scans the declaration
> +    /// block, and record an action for `update` to take. See document
> +    /// of `DeclarationUpdate` for meaning of actions it produces.

For the meaning of each variant, maybe?

::: servo/components/style/properties/declaration_block.rs:592
(Diff revision 1)
> +    ///
> +    /// Otherwise, for each new declaration, it scans the declaration
> +    /// block, and record an action for `update` to take. See document
> +    /// of `DeclarationUpdate` for meaning of actions it produces.
> +    ///
> +    /// For declarations don't override anything, it yields an `Append`.

nit: For declarations that don't override anything.

::: servo/components/style/properties/declaration_block.rs:598
(Diff revision 1)
> +    ///
> +    /// If a declaration of the same property exists, it checks whether
> +    /// it belongs to any logical group, and if so, it would check
> +    /// declarations after the existing one. If there is any declaration
> +    /// whose property is in the same logical group, but has different
> +    /// kind (logical / physical), an `AppendAndRemove` is yielded.

Maybe:

If a declaration for the same longhand exists, it checks whether there is any longhand with the same logical group but different kind (logical / physical) at a later position in the declaration block. If so, `AppendAndRemove` is yielded, otherwise, either `Update` or `None` is returned.

::: servo/components/style/properties/declaration_block.rs:603
(Diff revision 1)
> +    /// kind (logical / physical), an `AppendAndRemove` is yielded.
> +    ///
> +    /// Otherwise, if the declaration is identical to the existing one,
> +    /// `None`, otherwise, `Update`.
> +    ///
> +    /// See also Implementation details of `update` function below.

After reading this comment, I'm not sure it's really useful. It's pretty much a human translation of the code below, which I think it's pretty understandable, so if it was me I'd avoid all the `# Implementation details` section.

::: servo/components/style/properties/declaration_block.rs:653
(Diff revision 1)
> +                                return DeclarationUpdate::None;
> +                            }
> +                            return DeclarationUpdate::Update { pos };
> +                        }
> +                        if !needs_append {
> +                            if id.logical_group() == Some(logical_group) &&

nit: You can merge this condition with the one above: `if !needs_append && ...`.

::: servo/components/style/properties/declaration_block.rs:673
(Diff revision 1)
> +                    } else {
> +                        DeclarationUpdate::Update { pos }
> +                    }
> +                })
> +        }).inspect(|update| {
> +            if !matches!(update, DeclarationUpdate::None) {

```
if matches!(update, DeclarationUpdate::None) {
    return;
}
```

Then deindent the rest?

::: servo/components/style/properties/declaration_block.rs:691
(Diff revision 1)
> +        any_update
> +    }
> +
> +    /// Update this declaration block with the given data.
> +    ///
> +    /// # Implementation details

Similarly as above, I think I'd remove the comment, but if you think it's worth I wouldn't oppose keeping it.

::: servo/components/style/properties/declaration_block.rs:734
(Diff revision 1)
> +                }
> +            }
> +            return;
> +        }
> +
> +        self.declarations.reserve(updates.new_count);

Should we add `removal_count` and just `reserve()` `new_count - removal_count`?

::: servo/components/style/properties/declaration_block.rs:742
(Diff revision 1)
> +            struct UpdateOrRemoval<'a> {
> +                item: &'a mut DeclarationUpdate,
> +                pos: usize,
> +                remove: bool,
> +            }
> +            const LEN: usize = MAX_SUB_PROPERTIES_PER_SHORTHAND_EXCEPT_ALL;

Probably a type alias like:

```
type SubpropertiesVec<T> = ArrayVec<[T; MAX_SUB_PROPERTIES_PER_SHORTHAND_EXCEPT_ALL]>;
```

Would make this cleaner.

::: servo/components/style/properties/declaration_block.rs:756
(Diff revision 1)
> +                }).collect();
> +            // Execute removals. It's important to do it in reverse index order,
> +            // so that removing doesn't invalidate following positions.
> +            updates_and_removals.sort_unstable_by_key(|update| update.pos);
> +            for update in updates_and_removals.iter().rev() {
> +                if update.remove {

Maybe filter(|update| update.remove) instead of this condition, though no big deal.

::: servo/components/style/properties/declaration_block.rs:762
(Diff revision 1)
> +                    self.declarations.remove(update.pos);
> +                    self.declarations_importance.remove(update.pos);
> +                }
> +            }
> +            // Fixup pos field for updates.
> +            let mut offset = 0;

Maybe `declarations_already_removed` or something like that?

::: servo/components/style/properties/declaration_block.rs:765
(Diff revision 1)
> +            }
> +            // Fixup pos field for updates.
> +            let mut offset = 0;
> +            for update in updates_and_removals.iter_mut() {
> +                if update.remove {
> +                    offset += 1;

I'd continue here and deindent the rest, but not a big deal in any case.

::: servo/components/style/properties/declaration_block.rs:778
(Diff revision 1)
> +                    };
> +                }
> +            }
> +        }
> +        // Execute updates and appends.
> +        for (decl, update) in drain.declarations.zip(updates.updates.iter()) {

nit: We should assert that we're not dropping updates on the floor.

::: servo/components/style/properties/properties.mako.rs:2318
(Diff revision 1)
>  }
>  
> +/// An enum describes how a declaration should update
> +/// the declaration block.
> +#[derive(Clone, Copy, Debug, Eq, PartialEq)]
> +enum DeclarationUpdate {

This should probably move to declaration_block.rs

::: servo/components/style/properties/properties.mako.rs:2324
(Diff revision 1)
> +    /// The given declaration doesn't update anything.
> +    None,
> +    /// The given declaration is new, and should be append directly.
> +    Append,
> +    /// The given declaration can be updated in-place at the given position.
> +    Update { pos: usize },

WDYT of renaming this to `UpdateInPlace`? I think since all the variants are an `Update`, it'd be clearer this way.
Attachment #8992308 - Flags: review?(emilio) → review+
Comment on attachment 8992308 [details]
Bug 1473180 part 2 - Add new algorithm for setting property to be used in later commit.

https://reviewboard.mozilla.org/r/257176/#review264334

> After reading this comment, I'm not sure it's really useful. It's pretty much a human translation of the code below, which I think it's pretty understandable, so if it was me I'd avoid all the `# Implementation details` section.

If you think the code is understandable enough, I can remove all the implementation details. I wrote them down just in case it isn't clear enough.

> Should we add `removal_count` and just `reserve()` `new_count - removal_count`?

new_count only counts Append, no AppendAndRemove, so I don't think so. That's also why I called it `new_count` rather than `append_count`.
Comment on attachment 8992310 [details]
Bug 1473180 part 4 - Remove DeclarationPushMode::Update and related code.

https://reviewboard.mozilla.org/r/257180/#review264440

I think all the remaining callers are internal and don't care about appending vs. updating in-place. Should we just use the update-in-place mechanism instead given it's a bit faster? Not a big deal either way.
Attachment #8992310 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)
> Comment on attachment 8992310 [details]
> Bug 1473180 part 4 - Remove DeclarationPushMode::Update and related code.
> 
> https://reviewboard.mozilla.org/r/257180/#review264440
> 
> I think all the remaining callers are internal and don't care about
> appending vs. updating in-place. Should we just use the update-in-place
> mechanism instead given it's a bit faster? Not a big deal either way.

Actually, given all of them just use Normal, I think we can make push function unconditionally use the parsing path and continue using push for those callers. I believe in majority of case, we would just end up pushing directly.
Comment on attachment 8992309 [details]
Bug 1473180 part 3 - Use the new algorithm for setting property.

https://reviewboard.mozilla.org/r/257178/#review264338

Sorry for the lag here, wanted to look at the tests with care. Thanks!

::: testing/web-platform/meta/MANIFEST.json:327032
(Diff revision 1)
>       {}
>      ]
>     ],
> -   "css/cssom/cssstyledeclaration-setter-order.html": [
> +   "css/cssom/cssstyledeclaration-setter-declarations.html": [
>      [
> -     "/css/cssom/cssstyledeclaration-setter-order.html",
> +     "/css/cssom/cssstyledeclaration-setter-declarations.html",



::: testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-001.html:6
(Diff revision 3)
>  <!doctype html>
>  <meta charset="utf-8">
> -<title>CSSOM: CSSStyleDeclaration.setPropertyValue queues a mutation record when not actually mutated</title>
> +<title>CSSOM: CSSStyleDeclaration.setPropertyValue queues a mutation record when changed</title>
>  <link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setpropertyvalue">
>  <link rel="help" href="https://drafts.csswg.org/cssom/#update-style-attribute-for">
> -<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
> +<link rel="author" title="Emilio Cobos A?lvarez" href="mailto:emilio@crisal.io">

wow, my name was destroyed here :).

::: testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-001.html:19
(Diff revision 3)
>      assert_equals(r.length, 1);
>      test.done();
>    });
>  
>    m.observe(document.documentElement,  { attributes: true });
> -  document.documentElement.style.top = "0px";
> +  document.documentElement.style.top = "1px";

This looks fine though it certainly changes the purpose of the test.. Maybe we should add another to test that we _don't_ fire a mutation record if not changed? In any case looks fine, we're going to need to figure out what we want to do here.

::: testing/web-platform/tests/css/cssom/cssstyledeclaration-setter-declarations.html:26
(Diff revision 3)
> +    if (a.name < b.name) return -1;
> +    if (a.name > b.name) return 1;
> +    return 0;
> +  }
> +  function checkDeclarationsAnyOrder(block, props, msg) {
> +    let actual = [];

This may be more straight-forward comparing length with props.length, and calling getPropertyValue / getPropertyPriority instead of sorting... But no big deal.
Attachment #8992309 - Flags: review?(emilio) → review+
Comment on attachment 8992309 [details]
Bug 1473180 part 3 - Use the new algorithm for setting property.

https://reviewboard.mozilla.org/r/257178/#review264338

Sorry for the lag here, wanted to look at the tests with care. Thanks!

::: testing/web-platform/meta/MANIFEST.json:327032
(Diff revision 1)
>       {}
>      ]
>     ],
> -   "css/cssom/cssstyledeclaration-setter-order.html": [
> +   "css/cssom/cssstyledeclaration-setter-declarations.html": [
>      [
> -     "/css/cssom/cssstyledeclaration-setter-order.html",
> +     "/css/cssom/cssstyledeclaration-setter-declarations.html",



::: testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-001.html:6
(Diff revision 3)
>  <!doctype html>
>  <meta charset="utf-8">
> -<title>CSSOM: CSSStyleDeclaration.setPropertyValue queues a mutation record when not actually mutated</title>
> +<title>CSSOM: CSSStyleDeclaration.setPropertyValue queues a mutation record when changed</title>
>  <link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setpropertyvalue">
>  <link rel="help" href="https://drafts.csswg.org/cssom/#update-style-attribute-for">
> -<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
> +<link rel="author" title="Emilio Cobos A?lvarez" href="mailto:emilio@crisal.io">

wow, my name was destroyed here :).

::: testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-001.html:19
(Diff revision 3)
>      assert_equals(r.length, 1);
>      test.done();
>    });
>  
>    m.observe(document.documentElement,  { attributes: true });
> -  document.documentElement.style.top = "0px";
> +  document.documentElement.style.top = "1px";

This looks fine though it certainly changes the purpose of the test.. Maybe we should add another to test that we _don't_ fire a mutation record if not changed? In any case looks fine, we're going to need to figure out what we want to do here.

::: testing/web-platform/tests/css/cssom/cssstyledeclaration-setter-declarations.html:26
(Diff revision 3)
> +    if (a.name < b.name) return -1;
> +    if (a.name > b.name) return 1;
> +    return 0;
> +  }
> +  function checkDeclarationsAnyOrder(block, props, msg) {
> +    let actual = [];

This may be more straight-forward comparing length with props.length, and calling getPropertyValue / getPropertyPriority instead of sorting... But no big deal.
Comment on attachment 8992924 [details]
Bug 1473180 part 5 - Remove DeclarationPushMode.

https://reviewboard.mozilla.org/r/257752/#review264832

::: servo/components/style/properties/declaration_block.rs:472
(Diff revision 1)
>      ///
>      /// Depending on the value of `mode`, this has a different behavior in the
>      /// presence of another declaration with the same ID in the declaration
>      /// block.
>      ///
>      /// Returns whether the declaration has changed.

Please add to the doc comment pointing out that this is only used for parsing and internal use.
Attachment #8992924 - Flags: review?(emilio) → review+
Comment on attachment 8992309 [details]
Bug 1473180 part 3 - Use the new algorithm for setting property.

https://reviewboard.mozilla.org/r/257178/#review264338

> wow, my name was destroyed here :).

Ah, sorry about that. I removed the file initially, and later I copy-pasted it back. Probably my console or vim doesn't handle that correctly :/

> This looks fine though it certainly changes the purpose of the test.. Maybe we should add another to test that we _don't_ fire a mutation record if not changed? In any case looks fine, we're going to need to figure out what we want to do here.

I intentionally want to avoid adding such test, because it is not required by the proposed change to the spec. Basically, always appending is still a conformant implementation based on the constraints, and that should always cause a mutation record being fired.

> This may be more straight-forward comparing length with props.length, and calling getPropertyValue / getPropertyPriority instead of sorting... But no big deal.

It's easier to just reuse the object comparing from the harness :)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7e1a360196
part 1 - Introduce a concept of logical group. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/b863c916b7e5
part 2 - Add new algorithm for setting property to be used in later commit. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd916a453f87
part 3 - Use the new algorithm for setting property. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/75345fe6e02d
part 4 - Remove DeclarationPushMode::Update and related code. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/d487d2a5bc3e
part 5 - Remove DeclarationPushMode. r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12065 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged
See Also: → 1395767
Blocks: 1395767
You need to log in before you can comment on or make changes to this bug.