Closed Bug 1418874 Opened 7 years ago Closed 6 years ago

Drop nsCSSScanner and CSSLexer

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: heycam, Assigned: xidorn)

References

Details

Attachments

(4 files)

Bug 1418874 part 1 - Use JS impl of CSS lexer instead of that from InspectorUtils in devtools.
59 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1418874 part 2 - Move ctor and dtor of AnimationEffect into cpp file.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1418874 part 3 - Remove CSSLexer and related stuff.
59 bytes, text/x-review-board-request
emilio
: review+
tromey
: review+
Details
Bug 1418874 part 4 - Remove nsCSSScanner.
59 bytes, text/x-review-board-request
emilio
: review+
Details
CSSLexer is a JS-exposed object for devtools to tokenize CSS text. We should make it use a Rust CSS parser object instead.
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)
(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)
See Also: → 1410184
(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)
No longer blocks: stylo-everywhere
Priority: -- → P3
Blocks: stylo-everywhere
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
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
If the platform lexer isn't exposed then dom/webidl/CSSLexer.webidl can also be removed.
Assignee: nobody → xidorn+moz
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 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 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 on attachment 8985561 [details] Bug 1418874 part 4 - Remove nsCSSScanner. https://reviewboard.mozilla.org/r/251174/#review257436 Nice :)
Attachment #8985561 - Flags: review?(emilio) → 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 > 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 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 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+
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.
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.
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.
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
(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.
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
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.
No longer depends on: 1410184
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: