1392562 - generateCssProperties is slow

Status: RESOLVED INVALID

(DevTools :: Inspector, defect, P3)

Description

[Alexandre Poirot [:ochameau]]

http://searchfox.org/mozilla-central/source/devtools/server/actors/css-properties.js#48-81
This method frequently appears in profiles:
  https://perfht.ml/2x85rCF

It takes 90ms on my VM and if you clear the filter on perf-html you can see it as a significant pause into inspector loading sequence.

Comment 1

[Alexandre Poirot [:ochameau]]

It seems to be mostly because of this code:
http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/devtools/server/actors/css-properties.js#56-67
    for (let type in CSS_TYPES) {
      if (safeCssPropertySupportsType(name, DOMUtils["TYPE_" + type])) {
        supports.push(CSS_TYPES[type]);
      }
    }

    // Don't send colors over RDP, these will be re-attached by the front.
    let values = DOMUtils.getCSSValuesForProperty(name);
    if (values.includes("aliceblue")) {
      values = values.filter(x => !colors.includes(x));
      values.unshift("COLOR");
    }

The color filter is so dumb, but I don't know how to optimize this...

Tom, Any idea how to tweak that? Should we also computed a static file??

Comment 2

[Tom Tromey :tromey]

(In reply to Alexandre Poirot [:ochameau] from comment #1)

> The color filter is so dumb, but I don't know how to optimize this...

This is a primitive form of compression.  The colors are "re-attached" on the client.
IIRC Greg tried this both ways and this was faster...?

> Tom, Any idea how to tweak that? Should we also computed a static file??

I think he also attempted this but there was no way to integrate this into
the build system.

The ideal is not to do any of this stuff at all but rather change the inspector to
query the actor.  However, this also proved to be intractable.

Two ideas come to mind.

One is, fix the inspector.  Like I said, it's hard, but maybe now there's the impetus.
This is the best route.

The other idea is pushing this compression idea into inIDOMUtils and have it emit
some special token into the results to mean "colors are valid here"; then continue
with the re-attachment in the client.  Gross!  But it would work.

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Tom Tromey :tromey from comment #2)
> (In reply to Alexandre Poirot [:ochameau] from comment #1)
> 
> > The color filter is so dumb, but I don't know how to optimize this...
> 
> This is a primitive form of compression.  The colors are "re-attached" on
> the client.
> IIRC Greg tried this both ways and this was faster...?

I was wondering if we could only change the filtering algorithm.
May be Array.filter is slower than for()?
May be there is super-smart way to filter out color... somehow?
Or get the native API to not return the color if that's easy for it to do?

> I think he also attempted this but there was no way to integrate this into
> the build system.
> 
> The ideal is not to do any of this stuff at all but rather change the
> inspector to
> query the actor.  However, this also proved to be intractable.
> 
> Two ideas come to mind.
> 
> One is, fix the inspector.  Like I said, it's hard, but maybe now there's
> the impetus.
> This is the best route.

Thanks for the history telling ;)

Yes it sounds hard, I tried to track down cssProperties, but by itself it already draws quite complex codepaths!

Comment 4

[Greg Tatum [:gregtatum]]

I don't really trust my old measurements, especially since that was before perf.html. Because of all the compile flags that go into the CSS system, it's impossible for our build system to generate a declarative list, so we have to ask. The biggest thing is getting the correct set of supported CSS properties on the given thing that we are inspecting. Is there a way for the actor to send over a version number for the build and then cache our database on the version? That would mean we compute it once and then re-use it.

Comment 5

[Greg Tatum [:gregtatum]]

I re-ran a test of the async loading in the front, doing runs one with each approach. They are really in the same range.

perf-html loading screen
Sending full colors: 108ms
Sending full colors: 70ms
Sending full colors: 58ms
Sending full colors: 81ms
Sending full colors: 87ms

Shaving off colors: 76ms
Shaving off colors: 64ms
Shaving off colors: 69ms
Shaving off colors: 67ms
Shaving off colors: 68ms

cnn.com
Sending full colors: 324ms
Sending full colors: 445ms
Sending full colors: 549ms
Sending full colors: 247ms

Shaving off colors: 159ms
Shaving off colors: 215ms
Shaving off colors: 233ms
Shaving off colors: 514ms
Shaving off colors: 441ms

Comment 6

[Greg Tatum [:gregtatum]]

Brad: This is something that platform knowledge may help us optimize. I figured I'd add it to your radar.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

it looks like we're not receiving the full list of named colors anymore, but directly the "COLOR" item that we swap in the client (added in https://hg.mozilla.org/mozilla-central/rev/83df94ad2416a8f603acdbdc9fccdfccea90ffba)