Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ComplexSelector to_css should probably not do recursion #16019

Closed
bzbarsky opened this issue Mar 18, 2017 · 14 comments
Closed

ComplexSelector to_css should probably not do recursion #16019

bzbarsky opened this issue Mar 18, 2017 · 14 comments

Comments

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Mar 18, 2017

Currently it does:

    if let Some((ref next, ref combinator)) = self.next {
        next.to_css(dest)?;
        combinator.to_css(dest)?;
    }

which will run into crashes due to running out of stack; see https://bugzilla.mozilla.org/show_bug.cgi?id=475215

@highfive
Copy link

@highfive highfive commented Mar 20, 2017

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@emilio
Copy link
Member

@emilio emilio commented Mar 20, 2017

Happy to mentor this one.

@jdm
Copy link
Member

@jdm jdm commented Mar 20, 2017

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Mar 20, 2017

Yes, that is the general idea. The complication is that you want to loop backwards over a singly linked list.

Gecko does the obvious thing here: use a growable array with some preallocated stack storage (so you don't have to heap-allocate in common cases), walk the linked list adding things to the array, then walk the array backwards in a loop.

@tictakk
Copy link

@tictakk tictakk commented Mar 21, 2017

Hi, I'm looking to contribute to servo. Is this being worked on/assigned?

@emilio
Copy link
Member

@emilio emilio commented Mar 21, 2017

Please take it if you want! Reply to this mentioning @highfive, or saying you want to take it, and we'll assign it to you :)

@tictakk
Copy link

@tictakk tictakk commented Mar 22, 2017

Okay, awesome. @highfive: assign me

@highfive
Copy link

@highfive highfive commented Mar 22, 2017

Hey @tictakk! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Mar 22, 2017
@bholley bholley added the A-stylo label Mar 26, 2017
@highfive
Copy link

@highfive highfive commented Mar 26, 2017

cc @emilio

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Mar 27, 2017

@tictakk Any news on this?

@tictakk
Copy link

@tictakk tictakk commented Mar 27, 2017

@wafflespeanut Spent the weekend finishing a school project and brushing up on Rust. I'll be expeditious as possible.

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Mar 27, 2017

Great!

@tictakk
Copy link

@tictakk tictakk commented Mar 28, 2017

@emilio Hey, I think I could use some of that mentorship you mentioned. Should I be creating a vector of each child and looping to_css() one at a time?

@emilio
Copy link
Member

@emilio emilio commented Mar 28, 2017

Yes, that sounds right. We could consider creating a SmallVec to avoid heap allocation in the common case :)

@tictakk tictakk mentioned this issue Apr 1, 2017
4 of 4 tasks complete
@tictakk tictakk mentioned this issue Apr 11, 2017
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Apr 17, 2017
Removing recursion from ComplexSelector

<!-- Please describe your changes on the following line: -->
Using a smallvec instead of recursively calling to_css for ComplexSelector to handle running out of stack.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #16019  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16347)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants
You can’t perform that action at this time.