Bugzilla

Assignee

Updated

•

6 years ago

Assignee: nobody → ttromey

Flags: needinfo?(ttromey)

Assignee

Comment 4

•

6 years ago

From irc:

<safwan> I toggled layout.css.servo.enabled in about:config and now its
	 working as expected
<safwan> So thinking its servo related issue


I confirmed this as well.

Assignee

Comment 5

•

6 years ago

With gecko's css the rule is line 11, column 66839.
With servo it is column 68189.

Assignee

Comment 6

•

6 years ago

The bug is that Servo computes column numbers using byte offset from the start of the line,
where the buffer is UTF-8 encoded.  Gecko, on the other hand, uses the character offset
from the start of the line, where the buffer is UTF-16 encoded.  So, any non-ASCII character
should cause bad behavior here.  It's worth noting that Gecko handles surrogates pretty
much the way JS does, which also rules out a more straightforward character-based approach
in Servo.

Assignee

Updated

•

6 years ago

Blocks: stylo-devtools

Comment 7

•

6 years ago

One option might be to have the JS code do the conversion: start from the start of the line, look at each UTF-16 code unit, count how many bytes it would take in UTF-8, and stop when reaching the given "column" number.

* 0x0000 to 0x007F: 1 byte
* 0x0080 to 0x07FF: 2 bytes
* 0x0800 to 0xD7FF: 3 bytes
* 0xD800 to 0xDFFF: 2 bytes (Assuming surrogates are paired correctly, a pair would take 4 bytes in UTF-8.)
* 0xE000 to 0xFFFF: 3 bytes


Also any of these byte sequences (written escaped) starts a new line in cssparser: \r\n, \r, \n, \f. Although cases where this makes a difference are probably less common, JS code that tries to extract a substring based on line/column numbers also need to have the same definition of "line".

Assignee

Comment 8

•

6 years ago

I looked into working around this bug on the devtools side.
There are several problems.

There's this bug.  This one could be handled by (1) noticing servo
(DOMWindowUtils.isStyledByServo), then re-computing the column number
per comment #7.

To handle source maps, we'll need to also do this work before applying
the source map.  This means fetching the style sheet text before source-mapping;
I was planning to avoid this (bug 1388855 and follow-ups), but we can also
restrict this fetching to stylo.

Another issue is that css warnings emitted to the console will have the
wrong column.  These warnings don't go through the style actor, so we'll
need extra code somewhere to fetch the source text before attempting source-mapping.
Again, this can be made stylo-specific.

We may want to update the displayed column in the non-source-mapping case,
which means fetching the style sheet text whenever such a warning is issued.

I've updated this to show the console problem:
https://tromey.github.io/source-map-examples/css-warning/index.html
Here, with stylo, the warning shows column 233 (it seems to be emitting
the warning using the location of the "color" token); but gecko shows
column 237 (it's using the location of the value).
Here, both are wrong (though gecko's way of being wrong is easier to deal with):
stylo is off by 2 due to the encoding, and gecko treats the surrogates as 2 characters
for the purposes of column reporting.


My main concern about implementing the workaround in devtools is that it is going
to make source mapping even slower.  It's also more complicated but I think I can
put comments all over to explain what is going on and why.


I also looked briefly at fixing this on the servo side.  I think the "character column"
could be tracked reasonably efficiently by adding some code to the tokenizer's
|advance| function (plus updates to |consume_char| and |consume_byte|).  I didn't look
to see how hard this is to wire up to everything else; or whether anything in servo
depends on the column being a byte offset.

Assignee

Comment 9

•

6 years ago

I tried out the rust cssparser change.  More involved than I thought (of course :) but not too bad so far.

Comment 10

•

6 years ago

By "character", do you mean Unicode code point? That’s better than UTF-8 or UTF-16 code units, but still does account for combining code points, for example. "Unicode grapheme clusters" are closer to what people think as "characters", but more expensive to compute. And "columns in a monospace font" is something else again, since some characters (for example in CJK scripts) are "wide" and take two columns.

And generally, the more accurate a metric for counting "characters" is from people’s point of view, the less appropriate it is for programmatically slicing a substring. I think these are conflicting goals, and we might be better off tracking two metrics separately.


As to cssparser’s |advance| function, it’s extremely hot. I’m afraid adding any computation that occurs for every byte or every code point of the input would have a large parsing performance impact.

J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)

Comment 11

•

6 years ago

> still does account for combining code points, for example.

Does not*

Updated

•

6 years ago

Summary: DOM inspector is missing some css declarations in Firefox Nightly 57 → Stylo: DOM inspector is missing some css declarations in Firefox Nightly 57

Patrick Brosset <:pbro>

Comment 12

•

6 years ago

Is this a duplicate of bug 1392446?

Assignee

Comment 13

•

6 years ago

(In reply to Patrick Brosset <:pbro> from comment #12)
> Is this a duplicate of bug 1392446?

Maybe, but since the discussion is happening here, if one is closed, I'd rather it be 1392446.

Assignee

Comment 14

•

6 years ago

(In reply to Simon Sapin (:SimonSapin) from comment #10)
> By "character", do you mean Unicode code point? That’s better than UTF-8 or
> UTF-16 code units, but still does account for combining code points, for
> example.

I mean UTF-16, though it's unclear whether the distinction between UTF-16 and
Unicode code points matters in practice.

There are two reasons I think UTF-16 is appropriate.

First, it's easier to deal with from JS.  This matters because much of the source map
tooling is in JS.  Spot checking a couple of transformers shows that they're
working with JS strings and not taking special care for code points > 0xffff.

Second, this choice is compatible with Gecko.  Of course this is changeable, but
such a change would require auditing devtools to be sure that the change is accounted
for.  (It might just be the style actor; but maybe the autocompleter and other client-side
CSS parsing would need to be examined as well.)

Now, why I say the distinction may not matter is that there are also non-JS transformers,
and I don't know what these decide is a "column".  It seems likely to me that there
are already discrepancies in source maps in the wild.

I think source maps are an important issue because it is the one code path in devtools
where we currently don't need the style sheet text in order to present something meaningful;
and continuing with a byte offset will force us to change this.

It would be ideal if the source map spec declared what it meant by "column" so that
eventually the generators could conform.

> And generally, the more accurate a metric for counting "characters" is from
> people’s point of view, the less appropriate it is for programmatically
> slicing a substring.

I don't think this matters; the issues are really around source maps and extracting
rule text from the style sheet.

> As to cssparser’s |advance| function, it’s extremely hot. I’m afraid adding
> any computation that occurs for every byte or every code point of the input
> would have a large parsing performance impact.

I will try to test it.

Assignee

Comment 15

•

6 years ago

According to cargo bench it is definitely slower.

Before

test tests::numeric      ... bench:  55,684,707 ns/iter (+/- 1,016,410)
test tests::unquoted_url ... bench:     117,828 ns/iter (+/- 6,191)


After

test tests::numeric      ... bench:  59,075,137 ns/iter (+/- 964,524)
test tests::unquoted_url ... bench:     208,433 ns/iter (+/- 7,475)

Assignee

Comment 16

•

6 years ago

FWIW patch is here: https://github.com/tromey/rust-cssparser/tree/utf-16-columns

Assignee

Comment 17

•

6 years ago

I have an idea for how to remove the new code from advance.
Maybe that will help.

Assignee

Comment 18

•

6 years ago

I got it down to:

test tests::numeric      ... bench:  58,933,178 ns/iter (+/- 2,043,726)
test tests::unquoted_url ... bench:     158,958 ns/iter (+/- 8,384)


Another idea might be to defer column computation until requested.

Assignee

Comment 20

•

6 years ago

(In reply to Tom Tromey :tromey from comment #18)

> Another idea might be to defer column computation until requested.

This doesn't slow down the tokenizer at all, and is rather simpler, but
it seems to me that whether it's better overall would depend on how often
it is called.

You can see this here: https://github.com/tromey/rust-cssparser/tree/utf-16-lazy-columns

test tests::numeric      ... bench:  55,875,716 ns/iter (+/- 2,481,605)
test tests::unquoted_url ... bench:     118,140 ns/iter (+/- 13,363)

Comment 21

•

6 years ago

(In reply to Tom Tromey :tromey from comment #20)
> (In reply to Tom Tromey :tromey from comment #18)
> 
> > Another idea might be to defer column computation until requested.
> 
> This doesn't slow down the tokenizer at all, and is rather simpler, but
> it seems to me that whether it's better overall would depend on how often
> it is called.
> 
> You can see this here:
> https://github.com/tromey/rust-cssparser/tree/utf-16-lazy-columns
> 
> test tests::numeric      ... bench:  55,875,716 ns/iter (+/- 2,481,605)
> test tests::unquoted_url ... bench:     118,140 ns/iter (+/- 13,363)

I think what you really want to be measuring is the gtest microbenchmarks we have in mozilla-central. So you probably want to be hacking on the vendored version of cssparser in m-c (Simon can show you how to do that).

Manish Goregaokar [:manishearth]

Assignee

Comment 22

•

6 years ago

Yet another option would be not to try to compute the column number until truly needed --
before calls to log_css_error, and in get_location_with_offset.

Also perhaps the tokenizer could cache the current offset/column, to maybe handle very long
lines a bit better.

Updated

•

6 years ago

Flags: needinfo?(manishearth)

Assignee

Comment 23

•

6 years ago

> I think what you really want to be measuring is the gtest microbenchmarks we have in mozilla-central. So you probably want to be hacking on the vendored version of cssparser in m-c

The results look better there.

Before:

  56.676 ±  0.928 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  37.895 ±  0.542 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 381.688 ±  3.582 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 557.172 ±  1.703 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench


After (a somewhat modified version of the patch from comment #16):

  56.758 ±  0.744 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  40.271 ±  0.325 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 381.704 ±  3.693 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 557.710 ±  1.279 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench

So maybe this way is ok after all?
Simon, could you comment on the numbers?  If that looks alright then I will clean up the patch
and send a PR.

Flags: needinfo?(simon.sapin)

Comment 24

•

6 years ago

I find it pretty surprising that the previous patch showed a huge impact and it shows no impact in Firefox. How much sanity checking have you done that your code is actually running? Are you patching the vendored source in third_party/rust with cargo-edit-locally, or something else?

I think we need to understand why the benchmarks report such different results.

Assignee

Comment 25

•

6 years ago

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #24)
> I find it pretty surprising that the previous patch showed a huge impact and
> it shows no impact in Firefox. How much sanity checking have you done that
> your code is actually running? Are you patching the vendored source in
> third_party/rust with cargo-edit-locally, or something else?

I applied my patch in third-party/rust/cssparser and rebuilt.

I know the new code is running because the original problem in this bug goes away.
(And I know I have servo enabled and that the site hasn't changed because I also
tried it with another build to verify that the problem still occurs.)

> I think we need to understand why the benchmarks report such different
> results.

I dug deeper and the results aren't actually better.
The way I approached it is to look at the added overhead: time difference divided by number
of bytes in the test.

The Servo_StyleSheet_FromUTF8Bytes_Bench uses a test sheet that is 58823 bytes.
This comes to 1.39 ns/byte.

The cssparser url benchmark uses a sheet that is 192445 bytes.
This comes to 0.2 ns/byte.

The cssparser numeric benchmark parses "10px" 1000000 in a loop.
Calling that 4000000 bytes makes it come to .05 ns/byte.

So I suppose what we're seeing is that the cssparser benchmarks are actually doing
better per byte (kind of makes sense given, in particular, the nature of the
unquoted url parsing code); and that I was confused by the difference in scale.


I'm investigating a bit more, since I'd prefer to salvage this approach.
So far, profiling has not been very helpful though.

Assignee

Comment 26

•

6 years ago

> The way I approached it is to look at the added overhead: time difference divided by number
of bytes in the test.

This seems fishy because there is a lot of variance in the baseline numbers.
That is, the gtest with an unmodified tree is 963 ns/byte, but the cssparser
numbers are 0.6 and 13.9.
So now I think that's kind of worthless.

I finally figured out how to get debuginfo for profiling, to try to get line number
information.  But even with this the results are not useful.

Assignee

Comment 27

•

6 years ago

I found a way to improve the performance of the unquoted_url benchmark:

test tests::numeric      ... bench:  59,388,823 ns/iter (+/- 2,623,013)
test tests::unquoted_url ... bench:     103,630 ns/iter (+/- 4,152)

Basically I made sure that the column-adjusting code is only ever run for
non-ASCII characters; which in a way is gaming the benchmark, though at the
same time not completely unreasonable. 

Also I had to #[inline(never)] the function doing this extra computation.
Removing the #[inline(never)], with no other change, yields:

test tests::unquoted_url ... bench:     324,106 ns/iter (+/- 5,119)


I think I also figured out what was wrong with the numeric benchmark:

test tests::numeric      ... bench:  56,127,651 ns/iter (+/- 1,921,561)

I believe the problem is that this benchmark is sensitive to startup costs,
because it is doing

        for _ in 0..1000000 {
            let mut input = ParserInput::new("10px");
            let mut input = Parser::new(&mut input);
            let _ = test::black_box(input.next());
        }

I had added an extra field to ParserInput, and simply removing this field and its initialization
brought the numbers back down.  The new field is no longer needed because the newest patch
just abuses current_line_start_position rather than introducing a new field.


I don't know if this is the way to go -- how common is non-ASCII in a CSS? -- but at least
I managed to satisfy my feeling that there was a way to make it work.

Comment 28

•

6 years ago

(In reply to Tom Tromey :tromey from comment #27)
> I don't know if this is the way to go -- how common is non-ASCII in a CSS?
> -- but at least
> I managed to satisfy my feeling that there was a way to make it work.

Nice idea! My sense is that it's not super-common (certainly all the CSS keywords and property name are ascii), but Simon would know better. How bad is the cliff? Is it per-stylesheet, per-line, or per-character?

Anyway, the thing we really care about right now is the gecko benchmarks. Is there a noticeable regression there with your new strategy? Might also be worth swapping in a few other stylesheets, like the one from facebook tp6.

Comment 29

•

6 years ago

Optimizing for ASCII makes sense to me. The CSS language itself is entirely within ASCII, and the CSS WG has a resolution to keep it that way. This is excluding author-defined identifiers and strings, but they are typically a small fraction of a stylesheet’s bytes if we exclude base64-encoded (and so also within ASCII) data: URLs.

Parser startup costs are significant for JS-based animations that have code like this in a loop: some_element.style.left = some_number + "px";

Regarding lazy counting, ideally would could also skip it when log_css_error is used but nobody is looking at devtools. Is it possibly to log an error in the console not as a string but as an object whose stringification is computed lazily?



Tom, modifying third-party/rust/ probably doesn’t work. We tell Cargo to use that directory as a replacement for crates.io, which is considered immutable (append-only). So I suspect that when you rebuilt, Cargo already had a compiled cssparser and did not check these source files for modifications.

Compiling Gecko with local modifications to crates that are normally from crates.io is unfortunately very annoying, sorry. http://doc.crates.io/specifying-dependencies.html#overriding-dependencies has a guide. Applied to Gecko, you need to:

* Have a clone of the rust-cssparser repo next to your clone of mozilla-central
* Use `grep 'cssparser ' toolkit/library/rust/Cargo.lock` to find out what version is used, and make sure rust-cssparser/Cargo.toml is at the exact same version
* Add a [replace] section to both "root" Cargo.toml files like in the diff below. Here again the version number needs to match exactly.
* Update both Cargo.lock files by running (cd toolkit/library/rust && cargo update -p style) && (cd toolkit/library/gtest/rust && cargo update -p style)
* Now when building you should see a line like this (with a filesystem path) somewhere in the output:  0:20.90    Compiling cssparser v0.19.5 (file:///home/simon/projects/rust-cssparser)
* Modifying source files there and rebuilding should now pick up your modifications.


When modifying layout/style/test/gtest/example.css, a full `./mach build` is needed. Running only `./mach gtest Stylo*` rebuilds Rust and C++ code as needed, but I couldn’t figure out how to make the build system do the same for this example.css file.


diff --git a/toolkit/library/gtest/rust/Cargo.toml b/toolkit/library/gtest/rust/Cargo.toml
index ae654a565059..2425fcc46dce 100644
--- a/toolkit/library/gtest/rust/Cargo.toml
+++ b/toolkit/library/gtest/rust/Cargo.toml
@@ -45,3 +45,6 @@ opt-level = 2
 rpath = false
 debug-assertions = false
 panic = "abort"
+
+[replace]
+"cssparser:0.19.5" = {path = "../../../../../rust-cssparser"}
diff --git a/toolkit/library/rust/Cargo.toml b/toolkit/library/rust/Cargo.toml
index 04720a081a6d..bb12f9721067 100644
--- a/toolkit/library/rust/Cargo.toml
+++ b/toolkit/library/rust/Cargo.toml
@@ -43,3 +43,6 @@ opt-level = 2
 rpath = false
 debug-assertions = false
 panic = "abort"
+
+[replace]
+"cssparser:0.19.5" = {path = "../../../../rust-cssparser"}

Flags: needinfo?(simon.sapin)

Comment 30

•

6 years ago

(In reply to Simon Sapin (:SimonSapin) from comment #29)
> Optimizing for ASCII makes sense to me. The CSS language itself is entirely
> within ASCII, and the CSS WG has a resolution to keep it that way. This is
> excluding author-defined identifiers and strings, but they are typically a
> small fraction of a stylesheet’s bytes if we exclude base64-encoded (and so
> also within ASCII) data: URLs.
> 
> Parser startup costs are significant for JS-based animations that have code
> like this in a loop: some_element.style.left = some_number + "px";
> 
> Regarding lazy counting, ideally would could also skip it when log_css_error
> is used but nobody is looking at devtools. Is it possibly to log an error in
> the console not as a string but as an object whose stringification is
> computed lazily?

I think that would probably be doable, as long as the object had enough information to do the computation. It presumably no longer has access to the full stylesheet. Does it have access to at least the entire line?

J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)

Assignee

Comment 31

•

6 years ago

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #28)

> Nice idea! My sense is that it's not super-common (certainly all the CSS
> keywords and property name are ascii), but Simon would know better. How bad
> is the cliff? Is it per-stylesheet, per-line, or per-character?

My current implementation adds a bit of overhead for bytes >= 0x80.  However,
it can be reduced down to some overhead for UTF-8 introduction bytes only
(that is, no overhead for continuation bytes or for ASCII bytes).

> Anyway, the thing we really care about right now is the gecko benchmarks. Is
> there a noticeable regression there with your new strategy? Might also be
> worth swapping in a few other stylesheets, like the one from facebook tp6.

Still looking at this.

(In reply to Simon Sapin (:SimonSapin) from comment #29)

> Regarding lazy counting, ideally would could also skip it when log_css_error
> is used but nobody is looking at devtools. Is it possibly to log an error in
> the console not as a string but as an object whose stringification is
> computed lazily?

I think adding a flag would slow it down.

So, FWIW, I have several possible plans for fixing this bug.

A. Have cssparser track columns gecko-style.  I think this is the cleanest approach,
   as it isolates the changes in a single module; I consider it a latent requirement
   of the css parser, though I appreciate that this is probably idiosyncractic.  Maybe
   worth pointing out that there's already a devtools specific hack in this area
   https://dxr.mozilla.org/mozilla-central/rev/4caca1d0ba0e35cbe57a88493ebf162aa2cb3144/servo/components/style/stylesheets/rule_parser.rs#586

B. If A is too slow, try computing the gecko-style column on demand.  This would involve
   a second pass over the style sheet source line; however it could possibly perform
   well enough given that not every location is needed.  Maybe a single-entry cache would
   help as well.

C. If B is too slow, try B but just for CSS warnings, not rule locations.  The idea here
   is that maybe it's acceptable for error reporting to be a bit slow.  This plan leaks the
   servo column numbers to devtools, which will need to be modified to handle this.

D. If C is too slow, change devtools everywhere.  This is my last resort since it introduces
   previously-unneeded steps to devtools.

:jdm mentioned an idea of doing the column computation on another thread.  I haven't
looked into this yet.

> Tom, modifying third-party/rust/ probably doesn’t work.

I'm reasonably sure it worked for me, because the original test started passing.
However, I'll follow the directions to see if something changes.  Thanks for the info.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #30)

> I think that would probably be doable, as long as the object had enough
> information to do the computation. It presumably no longer has access to the
> full stylesheet. Does it have access to at least the entire line?

Not currently.  At one time the error reporters had access to the text.
For full laziness a copy would have to be made.

Comment 32

•

6 years ago

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #30)
> I think that would probably be doable, as long as the object had enough
> information to do the computation. It presumably no longer has access to the
> full stylesheet. Does it have access to at least the entire line?

The source line text for CSS errors was removed for Stylo in bug 1381045 when :jdm was working on a different performance issue.  So, reviving the text to do lazy column computation could be worse overall (something to watch out for at least).

Assignee

Comment 33

•

6 years ago

Time for a status update.  

I've tried a few different twists on the approach I came up with in comment #27.
In particular I tried special-casing the 4-byte-sequence leader byte and continuation bytes;
and I tried special-casing only non-ASCII leader bytes.  I also tried a few permuatations
of #[inline] and #[inline(never)] on the new helper functions.

For these I only ran the cssparser benchmarks.  The results are fairly noisy; for example
the choice of inlining affects the results strongly, even when the method in question is
never called by the benchmark.  So, it's difficult to trust I've made the correct selection.

Nevertheless -- I followed the vendoring instructions in comment #29 with the winner of the above.
This has yielded a speedup over the status quo.

Current M-C:

  49.380 ±  1.672 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  37.660 ±  0.442 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 415.060 ±  2.696 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 413.994 ±  1.725 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench

With my patch:

  42.109 ±  1.919 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  38.147 ±  0.483 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 410.992 ±  6.078 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 412.831 ±  1.639 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench


I haven't yet tried any other style sheets, as requested in comment #28.  That is what I will do next.
I am not sure how to get the sheet in question but I did find: https://www.joshmatthews.net/myspacebench

Comment 34

•

6 years ago

Tom, are your patches online somewhere?

https://github.com/rwood-moz/talos-pagesets/ has the files used for tp6 benchmarks. For example, example.css could be replaced with the concatenation of tp6/tp6-facebook_files/*.css.

Assignee

Comment 35

•

6 years ago

(In reply to Simon Sapin (:SimonSapin) from comment #34)
> Tom, are your patches online somewhere?

Not yet.  I hope to clean them up and post them soon.
I won't be sending a PR until I write tests; because the patches add
some new coding requirements I think it's important that they be reasonably
complete.

> https://github.com/rwood-moz/talos-pagesets/ has the files used for tp6
> benchmarks. For example, example.css could be replaced with the
> concatenation of tp6/tp6-facebook_files/*.css.

Thank you.  I will try this tomorrow.


I tried the myspace patch from comment #33.

Before:

 437.446 ±  3.775 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
 313.825 ±  4.740 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 408.164 ±  1.558 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 407.212 ±  1.074 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench

After:

 373.757 ±  2.232 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
 312.736 ±  1.224 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 392.443 ± 29.763 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 392.112 ±  1.503 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench

So right now it's looking like my patch improves performance somewhat on ASCII style sheets
(the myspace sheet is not 100% ASCII; but it only has 11 non-ASCII characters out of 573926 bytes).

Assignee

Comment 36

•

6 years ago

The tp6 results are very noisy but it looks like the patch is a bit slower there.

before
 799.143 ±  9.587 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
 575.168 ±  3.068 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 418.655 ±  9.810 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 418.590 ±  1.235 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench

after
 801.202 ±  6.119 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
 578.983 ±  9.578 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 414.763 ±  1.245 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 415.385 ±  1.700 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench

I'll clean up the patch tomorrow and perhaps someone else could try the benchmark as well.

Assignee

Comment 37

•

6 years ago

I've cleaned up the patches and they are here:
https://github.com/tromey/rust-cssparser/commits/utf-16-columns

Comment 38

•

6 years ago

(In reply to Tom Tromey :tromey from comment #37)
> I've cleaned up the patches and they are here:
> https://github.com/tromey/rust-cssparser/commits/utf-16-columns

Looks good. Do you want to open a pull request?

Assignee

Comment 39

•

6 years ago

(In reply to Simon Sapin (:SimonSapin) from comment #38)
> (In reply to Tom Tromey :tromey from comment #37)
> > I've cleaned up the patches and they are here:
> > https://github.com/tromey/rust-cssparser/commits/utf-16-columns
> 
> Looks good. Do you want to open a pull request?

Will do.  First I want to check to be sure that the relevant cases are being tested,
but that shouldn't take too long.

Thanks for looking at this.

Assignee

Comment 40

•

6 years ago

I wrote some tests, but as discussed on irc, :jdm is going to try to reproduce the benchmark results.

Josh Matthews [:jdm]

Comment 41

•

6 years ago

I replaced example.css for the StyleSheet_FromUTF8Bytes with the concatenated stylesheets from tp6 facebook. I ran the stylo gtests five times each and looked at the spread for the benchmarks from before and after the rust-cssparser changes were applied and vendored into m-c. I can't see any meaningful slowdown in the results!

Assignee

Comment 42

•

6 years ago

> I can't see any meaningful slowdown in the results!

Awesome.  Thank you for doing this.
I've sent the PR: https://github.com/servo/rust-cssparser/pull/192

Comment 43

•

6 years ago

https://github.com/servo/servo/pull/18336 includes this change.

Assignee

Comment 44

•

6 years ago

Thanks Simon.

I think once that lands in M-C, I will use this bug to add an integration test.

Comment 45

•

6 years ago

https://hg.mozilla.org/mozilla-central/rev/18d453c2736c
https://hg.mozilla.org/mozilla-central/rev/8cb43bf606f8

Comment 46

•

6 years ago

NI Tom to land the integration test and close the bug.

Flags: needinfo?(manishearth) → needinfo?(ttromey)

Priority: P1 → P4

Comment hidden (mozreview-request)

This adds an integration test for the CSS rule column issue from bug
1390455.  The fix was landed in upstream rust-cssparser.

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

J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)

Assignee

Comment 48

•

6 years ago

Clearing NI.

Flags: needinfo?(ttromey)

Comment 49

•

6 years ago

mozreview-review

Comment on attachment 8904645 [details]
Bug 1390455 - regression test for CSS rule columns;

https://reviewboard.mozilla.org/r/176458/#review181920

Looks good to me, thanks for working on this!

Attachment #8904645 - Flags: review+

Gabriel [:gl] (ΦωΦ)

Comment 50

•

6 years ago

mozreview-review

Comment on attachment 8904645 [details]
Bug 1390455 - regression test for CSS rule columns;

https://reviewboard.mozilla.org/r/176458/#review181970

::: devtools/client/inspector/rules/test/browser_rules_non_ascii.js:14
(Diff revision 1)
> +
> +add_task(async function () {
> +  // Use a few 4-byte UTF-8 sequences to make it so the rule column
> +  // would be wrong when we had the bug.
> +  const html = `<style type="text/css">
> +/*

Attachment #8904645 - Flags: review?(gl) → review+

Assignee

Comment 51

•

6 years ago

Comment #50 got cut off because bugzilla can't handle some UTF-8 sequences in the patch.
It's readable in reviewboard though.