Closed Bug 1376884 Opened 7 years ago Closed 7 years ago

stylo: Check stack depth in invalidation machinery and re-enable limits

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(4 files)

Part 1 - Hook the recursive invalidation traversal up to the stack checker machinery. r=bholley
11.59 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Part 2 - Fix up stack limit sanity assertions. v1
8.34 KB, patch
jseward
: review+
Details | Diff | Splinter Review
Part 3 - Reenable tight stacks sizes. r=me
1.94 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Part 4 - Add a tweaked version of our existing invalidation stress test that runs parallel in adaptive mode. r=me
2.21 KB, patch
bholley
: review+
Details | Diff | Splinter Review 
This will allow us to use small stacks in bug 1376883.
Priority: -- → P2
NI emilio to do this when bug 1376883 lands, and also to check for other recursive stuff.
No longer blocks: 1376883
Depends on: 1376883
Flags: needinfo?(emilio+bugs)
Summary: stylo: make the invalidation passes use an iterator rather than recursion → stylo: Check stack depth in invalidation machinery
So, re. other recursive stuff, selector-matching is recursive in the following way:

  A selector like `* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *` will recurse `max(star_count, dom_depth)`.

So it's controlled by the user, just not dependent on only DOM shape, but also selectors.

Do we care about this one, Bobby? We could also track depth there, but that'd be kinda unfortunate (and affect correctness).
Flags: needinfo?(emilio+bugs) → needinfo?(bobbyholley)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> So, re. other recursive stuff, selector-matching is recursive in the
> following way:
> 
>   A selector like `* * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> * *` will recurse `max(star_count, dom_depth)`.
> 
> So it's controlled by the user, just not dependent on only DOM shape, but
> also selectors.
> 
> Do we care about this one, Bobby? We could also track depth there, but
> that'd be kinda unfortunate (and affect correctness).

Yeah, if we need to we can probably just store the base stack address on the MatchContext and do a depth check.

dbaron said on IRC though that he wouldn't expect a selector like that to require recursion. So it's worth seeing if we can fix that somehow.
Flags: needinfo?(bobbyholley)
MozReview-Commit-ID: ILblsins91P
Attachment #8901333 - Flags: review?(jseward)
MozReview-Commit-ID: GJkLyigMK34
Attachment #8901334 - Flags: review+
Expanding the scope of this bug a bit, see bug 1376883 comment 26.
Assignee: emilio+bugs → bobbyholley
Summary: stylo: Check stack depth in invalidation machinery → stylo: Check stack depth in invalidation machinery and re-enable limits
Attachment #8901332 - Attachment description: Part 1 - Hook the recursive invalidation traversal up to the stack checker machinery. → Part 1 - Hook the recursive invalidation traversal up to the stack checker machinery. r=bholley
Attachment #8901334 - Attachment description: Part 3 - Reenable tight stacks sizes. v1 → Part 3 - Reenable tight stacks sizes. r=me
Attachment #8901333 - Flags: review?(jseward) → review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd7ac7bc3fdf
Add a tweaked version of our existing invalidation stress test that runs parallel in adaptive mode. r=me
https://hg.mozilla.org/mozilla-central/rev/fd7ac7bc3fdf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: