reduce duplication between NonCustomPropertyId and nsCSSProps.cpp

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: froydnj, Assigned: emilio)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

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.
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 :)
Flags: needinfo?(emilio)
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?
(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.
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?
Assignee: nobody → emilio
Flags: needinfo?(emilio)
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.
This is the first part only, will be posting more patches.

Nathan, ni? for comment 4.
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
And remove gPropertyTable / kCSSRawProperties while at it.

Depends on D2514.
(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 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
Attachment #8995996 - Flags: review+
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
Attachment #8996006 - Flags: review+
Pushed by emilio@crisal.io:
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
https://hg.mozilla.org/mozilla-central/rev/8347f14c4683
https://hg.mozilla.org/mozilla-central/rev/cbaa8e0834ca
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.