1479450 - reduce duplication between NonCustomPropertyId and nsCSSProps.cpp

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Nathan Froyd [:froydnj]]

NonCustomPropertyId::name has a static [&'static str] table of CSS property names.  The same table is present in nsCSSProps.cpp (kCSSRawProperties).  We should try to reduce this down to a single table.  Bonus points if we make the shared table relocation-free, so we're not paying extra cost for relocations and non-shared data.

kCSSRawProperties also gets turned into a hash table, which is extra per-process data.  My possibly-wrong ignorance says that this hash table is not performance-sensitive; it would be nice if we just made the lookup use binary search or something, so we weren't constructing this extra hash table at runtime.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Well, kCSSRawProperties has a single use, plus it's inspector so no perf-sensitive, should be straight-forward to get rid of.

LookupProperty is a bit more delicate, but that and the whole property table things should probably go away as well and use the same codepath as the rest of the CSS parser (the Rust stuff). I happened to do most of the prerequisite work in bug 1478990 :)

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I think we have other bugs filed for this blocking stylo-everywhere or so... and I don't think this matters for content process size, does it?

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> I think we have other bugs filed for this blocking stylo-everywhere or so...
> and I don't think this matters for content process size, does it?

It matters a small amount, because the static tables themselves (on the Rust and C++ side) are subject to relocations in their current form.  Such tables are in general not shared between processes (though they are shared on Windows, AIUI?).  nsCSSProps.cpp:gPropertyTable is also per-process data which I think is not really necessary, given an appropriate static table.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Yeah, we may as well clean this stuff up a bit.

(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> > I think we have other bugs filed for this blocking stylo-everywhere or so...
> > and I don't think this matters for content process size, does it?
> 
> It matters a small amount, because the static tables themselves (on the Rust
> and C++ side) are subject to relocations in their current form.  Such tables
> are in general not shared between processes (though they are shared on
> Windows, AIUI?).  nsCSSProps.cpp:gPropertyTable is also per-process data
> which I think is not really necessary, given an appropriate static table.

How can we avoid this Nathan?

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Implement nsCSSProps::LookupProperty using Rust. r=xidorn

Always assume allowed-for-all-content. There are a couple callers which weren't
doing that:

 * A unit test -> removed.

 * ComputeAnimationDistance: Used for testing (in transitions_per_property), and
   for the animation inspector. The animation inspector shouldn't show
   non-enabled properties. The transitions_per_property test already relies on
   getComputedStyle stuff which only uses eForAllContent.

 * GetCSSImageURLs: I added this API for the context menu page and such. It
   doesn't rely on non-enabled-everywhere properties, it was only using
   eInChrome because it was a ChromeOnly API, but it doesn't really need this.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

This is the first part only, will be posting more patches.

Nathan, ni? for comment 4.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > It matters a small amount, because the static tables themselves (on the Rust
> > and C++ side) are subject to relocations in their current form.  Such tables
> > are in general not shared between processes (though they are shared on
> > Windows, AIUI?).  nsCSSProps.cpp:gPropertyTable is also per-process data
> > which I think is not really necessary, given an appropriate static table.
> 
> How can we avoid this Nathan?

You want to have pointer-free data structures.  So for e.g. kCSSRawProperties, one way to approach this is to turn:

const char* const kCSSRawProperties[] = {...};

into a array with character data only, and then a separate table of indices into the character data:

const char kCSSRawPropertyNames[] = "...";
// uint16_t indices if you can get away with them.
const uint16_t kCSSRawPropertyIndices[] = { ... };

where:

&kCSSRawPropertyNames[kCSSRawPropertyIndices[i]] == kCSSRawProperties[i]

This is probably only feasible if you can get rid of nsCSSProps.cpp:gPropertyTable at the same time, perhaps by making nsCSSProps::LookupProperty binary search through kCSSRawProperty{Names,Indices}.

For the Rust side, you can see:

https://github.com/servo/rust-url/pull/291/commits/12a92d7d76929718df06b11494e1f4a73ba9c82f

for the application of a similar idea.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Convert GetStringValue to use Servo.

And remove gPropertyTable / kCSSRawProperties while at it.

Depends on D2514.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> > (In reply to Nathan Froyd [:froydnj] from comment #3)
> > > It matters a small amount, because the static tables themselves (on the Rust
> > > and C++ side) are subject to relocations in their current form.  Such tables
> > > are in general not shared between processes (though they are shared on
> > > Windows, AIUI?).  nsCSSProps.cpp:gPropertyTable is also per-process data
> > > which I think is not really necessary, given an appropriate static table.
> > 
> > How can we avoid this Nathan?
> 
> You want to have pointer-free data structures.  So for e.g.
> kCSSRawProperties, one way to approach this is to turn:
> 
> const char* const kCSSRawProperties[] = {...};
> 
> into a array with character data only, and then a separate table of indices
> into the character data:
> 
> const char kCSSRawPropertyNames[] = "...";
> // uint16_t indices if you can get away with them.
> const uint16_t kCSSRawPropertyIndices[] = { ... };
> 
> where:
> 
> &kCSSRawPropertyNames[kCSSRawPropertyIndices[i]] == kCSSRawProperties[i]
> 
> This is probably only feasible if you can get rid of
> nsCSSProps.cpp:gPropertyTable at the same time, perhaps by making
> nsCSSProps::LookupProperty binary search through
> kCSSRawProperty{Names,Indices}.
> 
> For the Rust side, you can see:
> 
> https://github.com/servo/rust-url/pull/291/commits/
> 12a92d7d76929718df06b11494e1f4a73ba9c82f
> 
> for the application of a similar idea.

kCSSRawPropertyNames is gone with the last patch, I'll look into the rust bits.

Comment 10

[Phabricator Automation]

Comment on attachment 8995996 [details]
Implement nsCSSProps::LookupProperty using Rust. r=xidorn

Xidorn Quan [:xidorn] UTC+10 has approved the revision.

https://phabricator.services.mozilla.com/D2514

Comment 11

[Phabricator Automation]

Comment on attachment 8996006 [details]
Convert GetStringValue to use Servo.

Xidorn Quan [:xidorn] UTC+10 has approved the revision.

https://phabricator.services.mozilla.com/D2516

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8347f14c4683
Implement nsCSSProps::LookupProperty using Rust. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbaa8e0834ca
Convert GetStringValue to use Servo. r=xidorn

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8347f14c4683
https://hg.mozilla.org/mozilla-central/rev/cbaa8e0834ca