Closed Bug 1265787 Opened 4 years ago Closed 4 years ago

replace inIDOMUtils.getCSSLexer

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.1 - May 9
Tracking Status
firefox49 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file)

Replace uses of inIDOMUtils.getCSSLexer for devtools de-chrome-ification project.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
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.
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.
Attachment #8747252 - Flags: review?(pbrosset) → review+
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!
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
(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.
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
Flags: needinfo?(ttromey)
Keywords: checkin-needed
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/
Attachment #8747252 - Attachment description: MozReview Request: Bug 1265787 - add javascript CSS lexer to devtools; r?pbro → MozReview Request: Bug 1265787 - add javascript CSS lexer to devtools; r=pbro
Rebased.
Flags: needinfo?(ttromey)
Keywords: checkin-needed
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.
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!
Flags: needinfo?(ttromey)
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.
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.
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
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/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0e4859aa979d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.