Simon Sapin (:SimonSapin) Assignee
	Description • 7 years ago

      No description provided.

	Simon Sapin (:SimonSapin) Assignee
	Comment 2 • 7 years ago

These micro-benchmarks can be run with:

    ./mach gtest Stylo.*

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

	Nathan Froyd [:froydnj]
	Comment 6 • 7 years ago
mozreview-review

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?

	Bobby Holley (:bholley)
	Comment 7 • 7 years ago
mozreview-review

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?

	Simon Sapin (:SimonSapin) Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

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.

	Simon Sapin (:SimonSapin) Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

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.)

	Bobby Holley (:bholley)
	Comment 10 • 7 years ago

(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.

	Simon Sapin (:SimonSapin) Assignee
	Comment 14 • 7 years ago
mozreview-review

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.

	Pulsebot
	Comment 16 • 7 years ago

Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b3007db548b
stylo: Add gtest microbenchmarks for CSS parsing performance. r=bholley,froydnj

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 7 years ago
bugherder

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