Closed Bug 1425700 Opened 2 years ago Closed 2 years ago

CSS property use counters not recorded when Stylo is enabled

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: heycam, Assigned: emilio)

References

Details

Attachments

(5 files, 1 obsolete file)

Since we haven't added something equivalent to the SetDocumentAndPageUseCounter call in nsCSSExpandedDataBlock::DoTransferFromBlock to the Servo CSS parser, we're probably not correctly counting usage of CSS properties listed in UseCounters.conf.  (Although currently we only have 'fill' and 'fill-opacity' listed there, and I only added those initially for testing.)
This is only during parsing right? If so it shouldn't be particularly hard.
Priority: -- → P1
(And we probably should add a test to this so that we can catch this regression earlier next time if any.)
Priority: P1 → P3
I think we should make these work :)
Assignee: nobody → emilio
As simple as I could make it, for now. We can improve on this.

I'll double-check Talos, too, in case the cost of the relaxed atomic ops are too high?

Though I certainly hope not.
Still not hooked into telemetry, I talked with :janerik and :gfritzsche about
that, but test incoming!

This intentionally doesn't handle CSSOM and such for now, will file followups
for those, though should be trivial.

I want to unify / clean up how we do the use counters and the error reporting
stuff for CSSOM, since the current function call still shows up in profiles,
but that should be a follow-up.
Mostly testing that they work, and that they record what we expect them to
record, that is, the actual property that was parsed, and not the properties
that it'd resolve or expand to.

That may be another tricky part for CSSOM, I think style setters would fail an
alias test if implemented with the current setup (we do the property lookup in
C++).
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4) 
> I'll double-check Talos, too, in case the cost of the relaxed atomic ops are
> too high?

Also try locally profiling [1].

Another way to solve the problem without atomic ops is to have TLS and merge at the end, like we do with the traversal. Might add some overhead though.

[1] https://github.com/rwood-moz/talos-pagesets/blob/master/tp6/tp6-facebook.html
(In reply to Bobby Holley (:bholley) from comment #7)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #4) 
> > I'll double-check Talos, too, in case the cost of the relaxed atomic ops are
> > too high?
> 
> Also try locally profiling [1].
> 
> Another way to solve the problem without atomic ops is to have TLS and merge
> at the end, like we do with the traversal. Might add some overhead though.

Yeah, specially since you need to clear it beforehand and such, to ensure you don't record stuff from other documents. Also, merging would probably be too costly for CSSOM stuff... Though I guess we could use the document one directly there. It's a bit more complicated, though probably not too much? Anyway, we'll see.

Probably the easiest in this case is to add a benchmark like StyloParsingBench, but with use counters... Will give that a shot tomorrow.
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> Yeah, specially since you need to clear it beforehand and such, to ensure
> you don't record stuff from other documents. Also, merging would probably be
> too costly for CSSOM stuff... Though I guess we could use the document one
> directly there. It's a bit more complicated, though probably not too much?
> Anyway, we'll see.

Yeah, parsing is only OMT for <link rel="stylesheet">. There already quite a bit of work to marshal stuff across the various threads, so it seems reasonable to store some local use counter information and then applying it to the global state when the stylesheet is delivered to the main thread (assuming there aren't too many of these use counters).

But it's all moot if atomic ops are fast enough.
That way we have a simple baseline to measure overhead.

Haven't actually profiled that yet though ;)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)
> Created attachment 9002605 [details]
> Add a parsing benchmarks with use counters.
> 
> That way we have a simple baseline to measure overhead.
> 
> Haven't actually profiled that yet though ;)

Note that this testcase is single-threaded, and thus doesn't measure cache line contention in the multi-threaded parsing case. Certainly a good starting point, but if the numbers look ok we should also measure the impact when all the cores are parsing.
Comment on attachment 9002605 [details]
Add a parsing benchmarks with use counters.

Bobby Holley (:bholley) has approved the revision.
Attachment #9002605 - Flags: review+
Comment on attachment 9002580 [details]
Add a very simple use counter implementation.

Cameron McCormack (:heycam) (away 24 Aug) has approved the revision.
Attachment #9002580 - Flags: review+
Comment on attachment 9002583 [details]
Hook the use counters into StyleSheet parsing.

Cameron McCormack (:heycam) (away 24 Aug) has approved the revision.
Attachment #9002583 - Flags: review+
This was consistently faster in the benchmark (even when counters were disabled,
which was slightly suspicious, but...).

Anyway, it's not really much code, most of it is FFI copy-pasta.
Flags: needinfo?(emilio)
Comment on attachment 9002730 [details]
Make the counters non-atomic counters and merge afterwards.

Bobby Holley (:bholley) has approved the revision.
Attachment #9002730 - Flags: review+
I just had a meeting with emilio to discuss how to connect all of this to Telemetry.
We came up with 3 options (we talked about the implementation, this is the high level summary):

1. A single place for defining which CSS properties have use counters, in the style code. Telemetry will use the generated list to generate its data and export the relevant Telemetry IDs.
  Advantage: A single place for definition
  Disadvantage: Quite some build time magic, harder for the probe-service to extract what use counter histograms exist

2. Two places to define which CSS properties have use counters (UseCounters.conf & style code). Generated style code can easily map to Telemetry IDs.
  Advantage: No build time magic (easy for parsers to extract data)
  Disadvantages: 2 places for definition (we might be able to catch if something is missing at build time)

3. We enable use counters for all CSS properties. Telemetry can generate histograms from single source of CSS properties (in style code).
  Advantage: No build-time magic, no runtime checks if there should be a use counter
  Disadvantage: A lot more CSS counters at once.

From the Telemetry side none of the options currently require Telemetry core to have a Rust API.

:dietrich: What's the current status on the use counters? Are we planning to enable all CSS properties? If so, any timeframe? (If we know we're going with 3 anytime soon, we don't need to implement any other solution)

:gfritzsche: Do you see any problems with either solution?
Flags: needinfo?(gfritzsche)
Flags: needinfo?(dietrich)
Attached file Cleanup SVG mapped attribute parser. (obsolete) —
* Remove use counters since they're useless right now and I'm re-implementing
   them in bug 1425700 (I'll cleanup UseCounterFor and friends later).

 * Remove a useless condition that was guarded in an attrName->NamespaceID() !=
   kNamespace_None that can't be true, since we bail out for attribute names
   that aren't atoms.

 * Rename some stuff to be clearer (TakeDeclarationBlock instead of GetDeclarationBlock).

 * Don't allocate a new URLExtraData for each mapped attribute, cache them in
   the parser instead.
Attachment #9003245 - Attachment is obsolete: true
(In reply to Jan-Erik Rediger [:janerik] from comment #17)
>
> :dietrich: What's the current status on the use counters? Are we planning to
> enable all CSS properties? If so, any timeframe? (If we know we're going
> with 3 anytime soon, we don't need to implement any other solution)

We want to expand to all of CSS by the end of year, along with a number of other areas, but only on Nightly.

Once we ship the current set on release channel and have an understanding of client and server side costs, we'll evaluate how to expand the set of use-counters enabled there.
Flags: needinfo?(dietrich)
(In reply to Jan-Erik Rediger [:janerik] from comment #17)
> :gfritzsche: Do you see any problems with either solution?

Option 2) has the risk of the two definition files running out of sync (which can be a time drain for investigations etc.). Functionally though both 1) & 2) should both get us data out.

3) sounds like it ties into the larger project, later. If some use counters for CSS are needed now, going for 1) or 2) seem feasible.
Flags: needinfo?(gfritzsche)
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56f244940096
Cleanup SVG mapped attribute parser. r=dholbert
Backout by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b5cb64d690
Back out changeset 56f244940096 for landing with the wrong bug number. r=backout
Comment on attachment 9002585 [details]
Add a test for the use counters.

Cameron McCormack (:heycam) has approved the revision.
Attachment #9002585 - Flags: review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8efef28be72c
Add a very simple use counter implementation. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/544957218647
Hook the use counters into StyleSheet parsing. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3f9356906b
Add a test for the use counters. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/42822c922dcf
Add a parsing benchmarks with use counters. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5203d9dc33b
Make the counters non-atomic counters and merge afterwards. r=bholley
See Also: → 1487095
You need to log in before you can comment on or make changes to this bug.