Closed
Bug 1292729
Opened 8 years ago
Closed 8 years ago
stylo: Consider avoiding style traversal of text nodes entirely
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
3.97 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
This came up during an IRC conversation between me and Emilio.
Servo currently traverses and computes style for text nodes. It starts with a copy of the parent style, and then layout occasionally tweaks it (I believe).
For stylo, it's not clear to me that we need this, and I'm wondering if we can get away with just sharing the style context with the containing element. This would let us avoid attaching ServoNodeData to non-Element nodes (and thus save memory), and simplify change hint processing.
Assignee | ||
Comment 1•8 years ago
|
||
Cameron, any thoughts here?
I'm also a bit confused about the :-moz-text anonymous box that we use to resolve style for text frames. It looks like we don't have any rules matching it, and the comment even says as much. Does that mean we can just use the parent style? If so, what's the purpose of it? Just to give us a different style context for some reason?
Flags: needinfo?(cam)
So it's not the same style as the parent because all the non-inherited properties are their initial values rather than inherited values. And we actually depend on that, so we couldn't just go ahead and use the same style context.
We also hard-code (ResolveStyleForText / ResolveStyleForOtherNonElement) the fact that it doesn't match anything and don't even try running matching on that pseudo.
(I don't recall why we introduced the distinction between :-moz-text and :-moz-other-non-element, but I suspect it has to do with SVG text and heycam can answer.)
Assignee | ||
Comment 3•8 years ago
|
||
[19:01:28] <bholley> dbaron: oh, ok - I totally had that wrong then
[19:01:59] <bholley> dbaron: but we never tweak the style in any more interesting way than that?
[19:02:32] <bholley> dbaron: i.e. it seems like we could still avoid cascading in servo, and give them their own style context with an inherited version of the parent's computed values (which we could compute on the fly)
[19:03:29] bholley has to run
[19:03:37] <bholley> dbaron: thanks for the help!
[19:03:56] <emilio> bholley: we sort of do that already, the text node's style is only the inherited version from the parent. But yeah, computing it lazily from gecko should be doable, I guess
Assignee | ||
Comment 4•8 years ago
|
||
I guess it boils down to the question of whether the work to create an inherited version of the parent style is nontrivial enough to be worth doing in parallel at the expense of the memory cost on text nodes.
Comment 5•8 years ago
|
||
In servo inheriting style structs is as easy as bumping the refcount in the appropriate structs, either from the initial values or the parent style, so my guess is that it's sort of trivial and we'll have to measure whether the memory cost is worth it.
I'm more concerned about the fact that right now we have to compute change hints for text nodes, and stick them in the parent element's change hint, which is sort lame :/.
Ideally we should find a solution that addresses both problems.
Comment 6•8 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #2)
> (I don't recall why we introduced the distinction between :-moz-text and
> :-moz-other-non-element, but I suspect it has to do with SVG text and heycam
> can answer.)
We needed to be able to apply some special rules to munge writing-mode when using text-combine-upright in ruby text, but previously we had a single pseudo :-moz-non-element used for both text and for a couple of other things (like the nsFirstLineFrame that contained the things outside the ::first-line).
I think Emilio is right that the way we produce ComputedValues for text nodes is relatively cheap.
Flags: needinfo?(cam)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #6)
> (In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain
> patch) from comment #2)
> > (I don't recall why we introduced the distinction between :-moz-text and
> > :-moz-other-non-element, but I suspect it has to do with SVG text and heycam
> > can answer.)
>
> We needed to be able to apply some special rules
Do you mean via having a selector that matches them, or some other mechanism? I understood from dbaron's comment and from grepping that we don't have any selectors that match :-moz-text in the UA sheet (or in the tree).
Does that also mean that we need to munge the text style data in some sort of interesting way (i.e. that just inheriting values from the parent is not sufficient)?
Flags: needinfo?(cam)
Comment 8•8 years ago
|
||
Sorry, I meant rules in a more generic way. We munge writing-mode in nsStyleContext::ApplyStyleFixups for text nodes when we're in a text-combine-upright:all, and we didn't want to do that for other non-element style contexts. So yes, just inheriting is not sufficient in this case. I think it's the only time Gecko doesn't just inherit the inherited structs for a text node.
Flags: needinfo?(cam)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #8)
> Sorry, I meant rules in a more generic way. We munge writing-mode in
> nsStyleContext::ApplyStyleFixups for text nodes when we're in a
> text-combine-upright:all, and we didn't want to do that for other
> non-element style contexts. So yes, just inheriting is not sufficient in
> this case. I think it's the only time Gecko doesn't just inherit the
> inherited structs for a text node.
I see. I think we can/should probably move that logic into servo, and have a special accessor that allows us to get the style for text nodes (which is identical to Servo_InheritComputedValues, modulo that tweak).
Then the only question is whether we should do that computation eagerly (at the expense of the memory overhead of having a ServoNodeData pointer on non-element nodes). I guess that's something we can decide later when we're looking at memory overhead.
Assignee | ||
Updated•8 years ago
|
Blocks: stylo-memory
Assignee | ||
Comment 10•8 years ago
|
||
Note: I'm adding some traversal code that will need to change if we do this. Grep the source for this bug number.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> Note: I'm adding some traversal code that will need to change if we do this.
> Grep the source for this bug number.
Scratch that - I decided to just return them and let the servo traversal code handle them so that we can keep all that logic in one place.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 12•8 years ago
|
||
MozReview-Commit-ID: CroFtWpuIrO
Assignee | ||
Updated•8 years ago
|
Attachment #8804501 -
Flags: review?(ecoal95)
Assignee | ||
Comment 13•8 years ago
|
||
This enables us to stop traversing text nodes in servo. I'll have a PR up for that shortly.
Assignee: nobody → bobbyholley
Assignee | ||
Comment 14•8 years ago
|
||
MozReview-Commit-ID: CroFtWpuIrO
Assignee | ||
Updated•8 years ago
|
Attachment #8804501 -
Attachment is obsolete: true
Attachment #8804501 -
Flags: review?(ecoal95)
Assignee | ||
Updated•8 years ago
|
Attachment #8804534 -
Flags: review?(ecoal95)
Updated•8 years ago
|
Attachment #8804534 -
Flags: review?(ecoal95) → review+
Comment 15•8 years ago
|
||
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c446e42a269
Style text nodes on the main thread. r=heycam
Comment 16•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•