Closed
Bug 1265787
Opened 8 years ago
Closed 8 years ago
replace inIDOMUtils.getCSSLexer
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox49 fixed)
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.
Updated•8 years ago
|
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
Assignee | ||
Comment 1•8 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•8 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•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8747252 -
Flags: review?(pbrosset) → review+
Comment 4•8 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32f63d114515
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 6•8 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.
Comment 7•8 years ago
|
||
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•8 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 10•8 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.
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f54c4f5fd0ed
Keywords: checkin-needed
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/dbde8615b303
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9024c91dbd4a99e49597cd8a92557a18e7d0d30
Flags: needinfo?(ttromey)
Assignee | ||
Comment 15•8 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•8 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
Assignee | ||
Comment 18•8 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•8 years ago
|
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0e4859aa979d
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0e4859aa979d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•1 year ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•