Closed Bug 1390455 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect, P4)

57 Branch
defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: oyvind, Assigned: tromey)

References

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170813183258

Steps to reproduce:

On my local devserver, when devolping a page i try to inspect an element in Nightly and the inspect element CSS panel is missing some css declaration. I developed the page and know there are styles specified for that element.


Actual results:

When inspecting an element wich have styles, the css panels is completely empty. I have to switch back to stable Firefox release to inspect that element and then the styles are showing in the css panel.


Expected results:

css declarations for that element should show in the css panel.
Component: Untriaged → Developer Tools: Inspector
Thank you for the report.
Have you reproduced this problem on a public website by any chance? (so that we can investigate).
If not, are you able to provide the HTML/CSS code that shows this problem?
Also, when the problem occurs, can you look at the browser console (cmd+shift+J) and see if there are browser errors in there? If so, please paste them here as a comment. They could be useful to pinpoint the issue.
Flags: needinfo?(oyvind)
Hi.
You can reproduce this on our public dev server here http://lab.zpirit.no/xlbil/

Try to inspect the footer with class main-footer in nightly 57. that element clearly has a background but nothing showes in the css column.

The only error in the console are a warning about mixed content if using https.

But if you reload the page with the console open the debugger kicks in with a jquery exception, as addressed here
https://bugs.jquery.com/ticket/13881

This does not happen in Firefox.
Flags: needinfo?(oyvind)
Thank you. I was able to reproduce this issue thanks to your dev server.
I did see an error in the browser console:

Error: couldn't find start of the rule
Stack trace:
getRuleText@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1579:13
getAuthoredCssText/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1239:20
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
_handleResultValue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:386:7
_run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:318:13
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1080:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1797:15
receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7

If I open the style-editor and make any change to the content of style.css and then come back to the inspector, then the rules for the <footer> element are displayed correctly.

Tom: could you look at this please? The error is thrown in getRuleText in styles.js
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ttromey)
Priority: -- → P1
Assignee: nobody → ttromey
Flags: needinfo?(ttromey)
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.
With gecko's css the rule is line 11, column 66839.
With servo it is column 68189.
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.
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".
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.
I tried out the rust cssparser change.  More involved than I thought (of course :) but not too bad so far.
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.
> still does account for combining code points, for example.

Does not*
Summary: DOM inspector is missing some css declarations in Firefox Nightly 57 → Stylo: DOM inspector is missing some css declarations in Firefox Nightly 57
Is this a duplicate of bug 1392446?
(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.
(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.
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)
I have an idea for how to remove the new code from advance.
Maybe that will help.
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.
(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)
(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).
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.
Flags: needinfo?(manishearth)
> 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)
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.
(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.
> 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.
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.
(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.
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)
(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?
(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.
(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).
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
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.
(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).
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.
I've cleaned up the patches and they are here:
https://github.com/tromey/rust-cssparser/commits/utf-16-columns
(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?
(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.
I wrote some tests, but as discussed on irc, :jdm is going to try to reproduce the benchmark results.
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!
> 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
Thanks Simon.

I think once that lands in M-C, I will use this bug to add an integration test.
NI Tom to land the integration test and close the bug.
Flags: needinfo?(manishearth) → needinfo?(ttromey)
Priority: P1 → P4
Clearing NI.
Flags: needinfo?(ttromey)
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+
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+
Comment #50 got cut off because bugzilla can't handle some UTF-8 sequences in the patch.
It's readable in reviewboard though.
Comment on attachment 8904645 [details]
Bug 1390455 - regression test for CSS rule columns;

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

> Can we indent this appropriately. We also typically move the html out of the add_task as const TEST_URI for the rule tests.

The bug is finicky about columns.  However I've changed it a bit to make it read better.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53286dcc59e9
regression test for CSS rule columns; r=gl,jryans
https://hg.mozilla.org/mozilla-central/rev/53286dcc59e9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.