1265787 - replace inIDOMUtils.getCSSLexer

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Replace uses of inIDOMUtils.getCSSLexer for devtools de-chrome-ification project.

Comment 1

[Tom Tromey :tromey]

A few options -

https://github.com/CSSLint/parser-lib
May have some issues though.  It does newline normalization which might affect offset reporting.

http://glazman.org/JSCSSP/
Has what looks to be a JS translation of nsCSSScanner.
I don't know how much divergence there's been.

One further idea is to try to use emscripten on the platform scanner, or maybe on
Servo's.  The latter may have some impedance problems.

Comment 2

[Tom Tromey :tromey]

I've provisionally decided to just translate nsCSSScanner.cpp and CSSLexer.cpp to js myself.
This is pretty easy and will give us good compatibility and some hope of porting over future
changes as well.

Comment 3

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1265787 - add javascript CSS lexer to devtools; r=pbro

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

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8747252 [details]
MozReview Request: Bug 1265787 - add javascript CSS lexer to devtools; r=pbro

https://reviewboard.mozilla.org/r/49781/#review46701

Nice. I don't have a lot to say about this, since this is a conversion of the C++ version, and a nice drop-in replacement.
I guess the only thing that comes to mind is how well it performs compared to native. Should we be worried? Should we be running some perf tests before landing this change?
It's not like we have a choice for devtools.html anyway, but still, it might be nice to start looking into performance.

::: devtools/shared/css-lexer.js:5
(Diff revision 1)
> +// A CSS Lexer.  This file is a bit unusual -- it is a more or less
> +// direct translation of layout/style/nsCSSScanner.cpp and
> +// layout/style/CSSLexer.cpp into JS.  This implements the
> +// CSSLexer.webidl interface, and the intent is to try to keep it in
> +// sync with changes to the platform CSS lexer.  Due to this goal,
> +// this file violates some naming conventions and consequently locally
> +// disables some eslint rules.

Great comment, thanks!

Comment 5

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32f63d114515

Comment 6

[Tom Tromey :tromey]

(In reply to Patrick Brosset <:pbro> from comment #4)

> I guess the only thing that comes to mind is how well it performs compared
> to native. Should we be worried? Should we be running some perf tests before
> landing this change?
> It's not like we have a choice for devtools.html anyway, but still, it might
> be nice to start looking into performance.

I'll do some tests tomorrow.
I think it's fine to land it meanwhile.

Comment 7

[Carsten Book [:Tomcat]]

needs rebasing

1 out of 1 hunks FAILED -- saving rejects to file .eslintignore.rej
patching file devtools/client/shared/output-parser.js
Hunk #1 FAILED at 1
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/shared/output-parser.js.rej
patching file devtools/shared/moz.build
Hunk #1 FAILED at 35
1 out of 1 hunks FAILED -- saving rejects to file devtools/shared/moz.build.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8747252 [details]
MozReview Request: Bug 1265787 - add javascript CSS lexer to devtools; r=pbro

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

Comment 9

[Tom Tromey :tromey]

Rebased.

Comment 10

[Tom Tromey :tromey]

The all-js lexer is quite a bit slower.

I collected all the .css files from the tree into a single file:

find obj-x86_64-pc-linux-gnu/ -name '*.css'|xargs cat > /tmp/all.css

Then I added this as an xpcshell test:

const { TextDecoder, OS } = Cu.import("resource://gre/modules/osfile.jsm", {});

const PATH = "/tmp/all.css";

const jsLexer = require("devtools/shared/css-lexer");
const domutils = Components.classes["@mozilla.org/inspector/dom-utils;1"]
                           .getService(Components.interfaces.inIDOMUtils);

function lexOne(obj, str) {
  let start = (new Date).getTime();

  let lexer = obj.getCSSLexer(str);
  while (lexer.nextToken()) {
  }

  let end = (new Date).getTime();
  do_print("++++++++++++++++ DELTA = " + (end - start));
  ok(true, "FINISHED");
}

add_task(function* () {
  let css = yield OS.File.read(PATH, { encoding: "utf-8" });

  lexOne(domutils, css);
  lexOne(jsLexer, css);
});



From this I get the results:

 0:00.44 LOG: Thread-1 INFO "++++++++++++++++ DELTA = 270"
 0:05.31 LOG: Thread-1 INFO "++++++++++++++++ DELTA = 4865"

That is, the pure js version is ~20 times slower.

all.css is 1474677 bytes, so this works out to 303119 bytes per second for the js implementation.

I'm don't know what our target numbers should be.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f54c4f5fd0ed

Comment 12

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=df57a1d196dc seems one of this 2 changes caused https://treeherder.mozilla.org/logviewer.html#?job_id=9144862&repo=fx-team#L0-L8834

can you take a look, thanks!

Comment 13

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/dbde8615b303

Comment 14

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9024c91dbd4a99e49597cd8a92557a18e7d0d30

Comment 15

[Tom Tromey :tromey]

Retriggering in the old try run didn't show the problem.
I've rebased and started a new run to see if I can reproduce.
Couldn't reproduce locally.

Comment 16

[Tom Tromey :tromey]

It seems the failure was indeed caused by bug 1263404, and I didn't realize that one
had been backed out at the same time.
So, re-requesting checkin here.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 18

[Tom Tromey :tromey]

Comment on attachment 8747252 [details]
MozReview Request: Bug 1265787 - add javascript CSS lexer to devtools; r=pbro

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0e4859aa979d

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e4859aa979d