Closed
Bug 1418874
Opened 7 years ago
Closed 6 years ago
Drop nsCSSScanner and CSSLexer
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla62
People
(Reporter: heycam, Assigned: xidorn)
References
Details
Attachments
(4 files)
CSSLexer is a JS-exposed object for devtools to tokenize CSS text. We should make it use a Rust CSS parser object instead.
Reporter | ||
Comment 1•7 years ago
|
||
Tom, what's the current status of CSSLexer in devtools? Is it still used? I see in devtools/shared/css/lexer.js there is a translation of nsCSSScanner. Is that now used for everything? If so, and we can remove CSSLexer, that would mean I can avoid updating it to call into rust-cssparser. (Although it seems a little unfortunate to have a separate re-implementation of the CSS tokenizer, since it will be easy for it to drift out of sync with the one Gecko is using.)
Flags: needinfo?(ttromey)
Comment 2•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #1)
CCing :jryans because I know he's interested in this area.
> Tom, what's the current status of CSSLexer in devtools? Is it still used?
Yes, it is still used in a few spots:
https://dxr.mozilla.org/mozilla-central/search?q=getCSSLexer+regexp%3Adomutils&redirect=false
One of these spots is an important test that tries to ensure that
our JS translation of the lexer doesn't get out of sync with platform.
Anyway, the client side of devtools uses the translated lexer now, to support the
attempt to make devtools not rely on chrome code; but the server side is free
to continue using this API, and does; probably for performance though I don't really
recall.
> I see in devtools/shared/css/lexer.js there is a translation of
> nsCSSScanner. Is that now used for everything? If so, and we can remove
> CSSLexer, that would mean I can avoid updating it to call into
> rust-cssparser. (Although it seems a little unfortunate to have a separate
> re-implementation of the CSS tokenizer, since it will be easy for it to
> drift out of sync with the one Gecko is using.)
I think the ideal here would be to actually use the same lexer everywhere -- compiling
the rust lexer to web assembly for the client side. See bug 1410184.
Flags: needinfo?(ttromey)
Right, I am hopeful that a Rust to WASM translation of rust-cssparser will be sufficient for DevTools needs, but I hit a few snags due to with the current state of WASM tooling. There's been some active work in that area, so I'll make another attempt soon.
:heycam, if we don't have a WASM approach in place by the time you want to get rid of old style system stuff, maybe it would make sense to move this old C++ lexer to /devtools and stop updating it after that point? Then DevTools can pick up updates once the WASM translation is in place. I think this should be an okay middle ground to live with for a bit, since changes to nsCSSScanner are fairly rare.
Flags: needinfo?(cam)
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO Nov 22 - Dec 3) from comment #3)
> :heycam, if we don't have a WASM approach in place by the time you want to
> get rid of old style system stuff, maybe it would make sense to move this
> old C++ lexer to /devtools and stop updating it after that point? Then
> DevTools can pick up updates once the WASM translation is in place. I think
> this should be an okay middle ground to live with for a bit, since changes
> to nsCSSScanner are fairly rare.
OK. We can always keep nsCSSScanner around until you migrate to the WASM translation. I was hoping we would be able to remove it at the same time as all the rest of the old style system. Though it seems to be only around 7 KiB of code so it's not a big deal.
Flags: needinfo?(cam)
Reporter | ||
Updated•7 years ago
|
No longer blocks: stylo-everywhere
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Blocks: stylo-everywhere
Assignee | ||
Comment 5•7 years ago
|
||
Oh, okay, looks like the plan is to have inspector use WASM version of rust-cssparser rather than porting CSSLexer.
No longer blocks: stylo-everywhere
Assignee | ||
Comment 6•7 years ago
|
||
I guess we can repurpose this bug to handle the removal of those parts once bug 1410184 gets fixed.
Blocks: stylo-everywhere
Depends on: 1410184
Summary: replace CSSLexer's use of nsCSSScanner with Servo → Drop nsCSSScanner and CSSLexer
Comment 7•7 years ago
|
||
If the platform lexer isn't exposed then dom/webidl/CSSLexer.webidl can also be removed.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8985559 [details]
Bug 1418874 part 2 - Move ctor and dtor of AnimationEffect into cpp file.
https://reviewboard.mozilla.org/r/251170/#review257432
::: dom/animation/AnimationEffect.h:37
(Diff revision 1)
> {
> public:
> NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(AnimationEffect)
>
> - AnimationEffect(nsIDocument* aDocument, const TimingParams& aTiming)
> + AnimationEffect(nsIDocument* aDocument, const TimingParams& aTiming);
I think canaltinova fixed this in bug 1451289?
::: dom/animation/AnimationEffect.cpp:45
(Diff revision 1)
> + : mDocument(aDocument)
> + , mTiming(aTiming)
> +{
> +}
> +
> +AnimationEffect::~AnimationEffect()
nit: I'd still use `= default` here, if you need to do this change.
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8985559 [details]
Bug 1418874 part 2 - Move ctor and dtor of AnimationEffect into cpp file.
https://reviewboard.mozilla.org/r/251170/#review257430
Oh, this is what Brian did do initially but didn't actually (bug 1456394 comment 50). Because of unified build, maybe?
Attachment #8985559 -
Flags: review?(hikezoe) → review+
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8985560 [details]
Bug 1418874 part 3 - Remove CSSLexer and related stuff.
https://reviewboard.mozilla.org/r/251172/#review257434
r=me, assuming Tromey is fine with the first patch.
Attachment #8985560 -
Flags: review?(emilio) → review+
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8985561 [details]
Bug 1418874 part 4 - Remove nsCSSScanner.
https://reviewboard.mozilla.org/r/251174/#review257436
Nice :)
Attachment #8985561 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 16•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985559 [details]
Bug 1418874 part 2 - Move ctor and dtor of AnimationEffect into cpp file.
https://reviewboard.mozilla.org/r/251170/#review257432
> nit: I'd still use `= default` here, if you need to do this change.
I didn't even know we can do this... I probably need to dig a bit into this as I cannot understand how the compiler would work on some cases with this at the definition position.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8985558 [details]
Bug 1418874 part 1 - Use JS impl of CSS lexer instead of that from InspectorUtils in devtools.
https://reviewboard.mozilla.org/r/251168/#review257750
Thanks for the patch. This looks good to me. The only possible issue would be if it regresses performance, though I'm not sure whether this area is generally tested in talos.
Attachment #8985558 -
Flags: review?(ttromey) → review+
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8985560 [details]
Bug 1418874 part 3 - Remove CSSLexer and related stuff.
https://reviewboard.mozilla.org/r/251172/#review257752
Thanks for the patch. I think this is good, but there are a couple of small things to address before landing.
::: devtools/shared/tests/unit/xpcshell.ini
(Diff revision 2)
> skip-if = toolkit == 'android'
> support-files =
> exposeLoader.js
>
> [test_assert.js]
> -[test_csslexer.js]
Rather than remove the test entirely, I think it would be better to modify it to continue testing the CSS lexer that is implemented in JS.
::: dom/chrome-webidl/InspectorUtils.webidl
(Diff revision 2)
> [TreatNullAs=EmptyString] optional DOMString pseudo = "");
> unsigned long getRuleLine(CSSRule rule);
> unsigned long getRuleColumn(CSSRule rule);
> unsigned long getRelativeRuleLine(CSSRule rule);
> boolean hasRulesModifiedByCSSOM(CSSStyleSheet sheet);
> - [NewObject] CSSLexer getCSSLexer(DOMString text);
I couldnt seem to put a note on CSSLexer.webidl...
Anyway, the comments there were the only documentation of the contract followed by the lexer as used in devtools. So, it would be good to move them into devtools/shared/css/lexer.js.
Also I am not sure, but does removing webidl require a DOM peer's review?
Attachment #8985560 -
Flags: review?(ttromey) → review+
Comment 22•6 years ago
|
||
It would be interesting to push to try to see the impact on performance for this patch queue.
DAMP is running an inspector test against this page:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/pages/custom/inspector/index.html#49-61
It creates a lot of css rules. The two tests "custom.inspector.open" and "custom.inspector.reload" should reflect any improvement/regression made to the rule view.
You can push to try like, with the following syntax:
try: -b o -p linux64 -u none -t damp-e10s --rebuild-talos 6 --artifact
The JS implementation is already known to be a significant performance bottleneck in the inspector, so I wouldn't be surprised if it regress things when using it even more.
Assignee | ||
Comment 23•6 years ago
|
||
Nothing seems to cause problem on the top level: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f4f6777a13a1&newProject=try&newRevision=85063ede573d&framework=1
On the two Windows pgo platforms, subtest custom.inspector.manyrules.selectnode regresses by ~4%. Overall I guess it's not a big problem.
Assignee | ||
Comment 24•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8985560 [details]
Bug 1418874 part 3 - Remove CSSLexer and related stuff.
https://reviewboard.mozilla.org/r/251172/#review257752
> I couldnt seem to put a note on CSSLexer.webidl...
>
> Anyway, the comments there were the only documentation of the contract followed by the lexer as used in devtools. So, it would be good to move them into devtools/shared/css/lexer.js.
>
> Also I am not sure, but does removing webidl require a DOM peer's review?
Removing a non-web-exposing webidl doesn't require DOM peer's review in general I think, but there is a landing hook which can enforce that if necessary. We'll see.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•6 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e10c1e086d80
part 1 - Use JS impl of CSS lexer instead of that from InspectorUtils in devtools. r=tromey
https://hg.mozilla.org/integration/autoland/rev/d65d4848c69c
part 2 - Move ctor and dtor of AnimationEffect into cpp file. r=hiro
https://hg.mozilla.org/integration/autoland/rev/fd6530d0498b
part 3 - Remove CSSLexer and related stuff. r=emilio,tromey
https://hg.mozilla.org/integration/autoland/rev/ad44fc73cf4c
part 4 - Remove nsCSSScanner. r=emilio
Comment 30•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #23)
> Nothing seems to cause problem on the top level:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=f4f6777a13a1&newProject=try&newR
> evision=85063ede573d&framework=1
>
> On the two Windows pgo platforms, subtest
> custom.inspector.manyrules.selectnode regresses by ~4%. Overall I guess it's
> not a big problem.
Oh. That's interesting.
I didn't realized we were having such different noise factors between platforms.
If you look only at custom.inspector.manyrules.selectnode test over all platforms,
you can see that the standard deviation is very small (<0.5%) only on windows pgos and windows-7-opt.
And everywhere where the stddev is low, the regression is reported between 3 and 5%:
windows 10 pgo
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f4f6777a13a1&newProject=try&newRevision=85063ede573d&originalSignature=2e2b666b2c9746ca9fe1184efc6f182c330e8398&newSignature=2e2b666b2c9746ca9fe1184efc6f182c330e8398&filter=custom.inspector.manyrules.selectnode&framework=1
windows 7 pgo
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f4f6777a13a1&newProject=try&newRevision=85063ede573d&originalSignature=389a5b1ef37772bad22611e4d6237fffea35c9cb&newSignature=389a5b1ef37772bad22611e4d6237fffea35c9cb&filter=custom.inspector.manyrules.selectnode&framework=1
window 7 opt
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f4f6777a13a1&newProject=try&newRevision=85063ede573d&originalSignature=2fa549258445b035f7463afad97852e46b399a6d&newSignature=2fa549258445b035f7463afad97852e46b399a6d&filter=custom.inspector.manyrules.selectnode&showOnlyImportant=1&framework=1
So I can draw two conclusions here:
* the noise on many platforms (linux / non-pgo) disallow us from correctly reporting regression around 4%!
* your patch is very likely regressing inspector performance by 4% around a couple of interactions in the inspector.
This regression piles up on the already known impact of JS parser which was used elsewhere.
So it forces us to look into the JS implementation and make it significantly faster to recover from this regression.
Bug 1410184 has been opened to see if we can speed this up thanks to wasm,
but it is unclear if/when this experiment will be fulfilled, nor if it will be successful in term of performance.
Comment 31•6 years ago
|
||
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/3aa8a1bb3763
part 3 - Remove CSSLexer and related stuff. r=emilio,tromey,smaug
https://hg.mozilla.org/mozilla-central/rev/e5463979c345
part 4 - Remove nsCSSScanner. r=emilio
Comment 32•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e10c1e086d80
https://hg.mozilla.org/mozilla-central/rev/d65d4848c69c
https://hg.mozilla.org/mozilla-central/rev/3aa8a1bb3763
https://hg.mozilla.org/mozilla-central/rev/e5463979c345
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 33•6 years ago
|
||
Ah! Someone just reported this patch actually improved the performance of the inspector, by reducing a lot the memory usage of it. See bug 1410716 comment 10. The test script for the inspector must be missing some usecases/codepaths.
Comment 34•6 years ago
|
||
bugherder |
Updated•6 years ago
|
status-firefox60:
--- → wontfix
status-firefox61:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•