1355028 - stylo: Add gtest microbenchmark for CSS parsing performance

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Comment 1

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

These can be run with:

    ./mach gtest Stylo.*

Note that running `./mach build` is required after modifying example.css

Review commit: https://reviewboard.mozilla.org/r/128366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128366/

Comment 2

[Simon Sapin (:SimonSapin)]

These micro-benchmarks can be run with:

    ./mach gtest Stylo.*

Note that running `./mach build` is required after modifying example.css

Comment 3

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128366/diff/1-2/

Comment 4

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128366/diff/2-3/

Comment 5

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128366/diff/3-4/

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

https://reviewboard.mozilla.org/r/128366/#review130908

::: layout/style/test/gtest/StyloParsingBench.cpp:26
(Diff revision 4)
> +  auto data = new URLExtraData(
> +    NullPrincipalURI::Create(), nullptr, NullPrincipal::Create());

Do we want to free this value, since AFAICT from reading just this code, the servo side doesn't take ownership of the value?

::: layout/style/test/gtest/StyloParsingBench.cpp:44
(Diff revision 4)
> +    auto stylesheet = new CSSStyleSheet(eAuthorSheetFeatures, CORS_NONE, RP_No_Referrer);
> +    nsCSSParser parser(nullptr, stylesheet);

I *think* `nsCSSParser` takes ownership of the stylesheet, but it might be better to declare this as:

```
  RefPtr<CSSStyleSheet> stylesheet = ...
```

just so it's clearer who's destroying what.

::: layout/style/test/gtest/generate_example_stylesheet.py:3
(Diff revision 4)
> +    header = ("#define EXAMPLE_STYLESHEET \"%s\"" % css
> +        .replace("\\", "\\\\")
> +        .replace("\n", "\\n")
> +        .replace("\"", '\\"')

For my own edification, why are we taking the generated file approach rather than using raw strings?

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

https://reviewboard.mozilla.org/r/128366/#review131218

r=me with those and Nathan's comments addressed.

::: layout/style/test/gtest/StyloParsingBench.cpp:22
(Diff revision 4)
> +
> +#define PARSING_REPETITIONS 100
> +
> +#ifdef MOZ_STYLO
> +
> +static void servo_parsing_bench() {

Nit: These names should be camel-cased.

::: layout/style/test/gtest/StyloParsingBench.cpp:28
(Diff revision 4)
> +  for (int i = 0; i < PARSING_REPETITIONS; i++) {
> +    Servo_StyleSheet_FromUTF8Bytes(
> +      nullptr, nullptr, &css, eAuthorSheetFeatures, data
> +    ).Consume();
> +  }

It would be interesting to measure whether the results are basically identical between this approach and one where we call Servo_StyleSheet_FromUTF8Bytes once with the string chars repeated 100 times. Might be worth measuring and leaving a comment to that effect?

Comment 8

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

https://reviewboard.mozilla.org/r/128366/#review130908

> Do we want to free this value, since AFAICT from reading just this code, the servo side doesn't take ownership of the value?

Xidorn tells we this was a use-after-free (because the stylesheet would take a RefPtr of it, and when dropped free it since the ref count was 1) and that using "RefPtr<URLExtraData> data = …;" should fix it, I’ll do that.

> For my own edification, why are we taking the generated file approach rather than using raw strings?

I used a raw string literal at some point before submitting a patch, but even then the header file was generated with Python from a CSS file. The details of the CSS file are not that interesting, when modifying it one would typically replace it entirely with a different stylesheet from some website, not edit something in the middle.

With a raw string literal I couldn’t use `#define` because of newlines, so I had something like `const nsLiteralCString = NS_LITERAL_CSTRING(…)`. Now I can use the same `#define` with both `NS_LITERAL_CSTRING` and `NS_LITERAL_STRING`, the latter converting to UTF-16 at compile-time.

Comment 9

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

https://reviewboard.mozilla.org/r/128366/#review131218

> It would be interesting to measure whether the results are basically identical between this approach and one where we call Servo_StyleSheet_FromUTF8Bytes once with the string chars repeated 100 times. Might be worth measuring and leaving a comment to that effect?

I tried it (using `css * 100` in Python and removing the C++ loop) and measured no detectable difference. (The variation between runs without changing the code was greater.)

Comment 10

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #9)
> Comment on attachment 8856411 [details]
> Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.
> 
> https://reviewboard.mozilla.org/r/128366/#review131218
> 
> > It would be interesting to measure whether the results are basically identical between this approach and one where we call Servo_StyleSheet_FromUTF8Bytes once with the string chars repeated 100 times. Might be worth measuring and leaving a comment to that effect?
> 
> I tried it (using `css * 100` in Python and removing the C++ loop) and
> measured no detectable difference. (The variation between runs without
> changing the code was greater.)

Ok, that's a very nice indication that we don't have a lot of per-stylesheet overhead beyond what's actually in the sheet. Worth writing a comment to that effect.

Comment 11

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128366/diff/4-5/

Comment 12

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128366/diff/5-6/

Comment 13

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128366/diff/6-7/

Comment 14

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

https://reviewboard.mozilla.org/r/128366/#review131334

::: layout/style/test/gtest/StyloParsingBench.cpp:44
(Diff revisions 6 - 7)
>  
>  static void GeckoParsingBench() {
> +  // Don’t use NS_LITERAL_STRING to work around
> +  // "fatal error C1091: compiler limit: string exceeds 65535 bytes in length"
> +  // https://msdn.microsoft.com/en-us/library/f27ch0t1.aspx
> +  NS_ConvertUTF8toUTF16 css(NS_LITERAL_CSTRING(EXAMPLE_STYLESHEET));

This adds to the measured time, but only once outside of the "repeat a 100 times" loop.

Comment 15

[Simon Sapin (:SimonSapin)]

Comment on attachment 8856411 [details]
Bug 1355028 - stylo: Add gtest microbenchmarks for CSS parsing performance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128366/diff/7-8/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b3007db548b
stylo: Add gtest microbenchmarks for CSS parsing performance. r=bholley,froydnj

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b3007db548b