replace inIDOMUtils.getCSSLexer

RESOLVED FIXED in Firefox 49

Status

P1
enhancement
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 49
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

2 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8747252 [details]
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/
Attachment #8747252 - Flags: review?(pbrosset)
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!
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 6

2 years ago
(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
(Assignee)

Comment 8

2 years ago
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
(Assignee)

Comment 9

2 years ago
Rebased.
Flags: needinfo?(ttromey)
Keywords: checkin-needed
(Assignee)

Comment 10

2 years ago
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)
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
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
(Assignee)

Comment 18

2 years ago
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/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e4859aa979d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.