Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upConvert node serialization to a purely iterative algorithm. #17520
Conversation
highfive
commented
Jun 25, 2017
|
Is the Travis failure spurious? I'm not sure what this is about a manifest change. |
|
Never mind, I didn't realize the manifests contained hashes that need to be updated after changing tests. |
|
Just bumping this; can we get it merged? |
|
|
|
Sorry about the delay, the Mozilla all-hands was last week, we've been a bit swamped! Would it be possible to simplify this, by using an iterator whose enum BeginOrEndNode {
Begin(Root<Node>),
End(Root<Node>),
}this would avoid having to use the stack in |
|
Could possibly be simplified a bit. |
| node.children() | ||
| }; | ||
| fn start_element<S: Serializer>(node: &Node, serializer: &mut S) -> io::Result<()> { | ||
| let elem = node.downcast::<Element>().expect("Node should be an Element"); |
This comment has been minimized.
This comment has been minimized.
| // As we encounter elements, we push them to this stack with a count of their children. | ||
| // Every time through the following loop, we decrement the child count of the topmost element. | ||
| // When it gets to 0, it's time to close the node. | ||
| let mut open_stack: Vec<(Root<Node>, u32)> = vec![]; |
This comment has been minimized.
This comment has been minimized.
asajeffrey
Jul 4, 2017
Member
It would be nice if we could roll this stack into the iterator, rather than have it explicit.
|
@asajeffrey Is the intent to increase reuse? I can refactor for that in mind if that's the goal. |
|
I suspect the code will be more readable with a clean separation between iterating over the nodes and serializing them. I don't think there's much scope for reuse here. |
|
@asajeffrey This is one of those places where coroutines would be a godsend. |
|
That's the nicest I can make it. I need to rebase these commits together, if we like it. |
|
This Travis failure looks spurious to me, or at least unrelated to this PR. Can someone rerun it? |
|
That's #17594. Most PRs are showing it. |
|
This is looking a lot nicer! I think it can be simplified even further by something like: struct SerializationIterator {
stack: Vec<SerializationCommand>,
}then the iterator is something like (ignoring templates for the moment): impl Iterator for SerializationIterator {
type Item = SerializationCommand;
fn next(&mut self) -> Option<SerializationCommand> {
let result = self.stack.pop();
if let Some(SerializationCommand::OpenElement(element)) {
self.stack.push(SerializationCommand::CloseElement(element));
for child in element.children().iter().rev() {
match child.downcast::<Element>() {
Some(child) => self.stack.push(SerializationCommand::OpenElement(child)),
None => self.stack.push(SerializationCommand::SerializeNonelement(child)),
}
}
}
result
}
} |
asajeffrey
added
S-needs-code-changes
S-needs-squash
and removed
S-awaiting-review
S-needs-rebase
labels
|
Before I try to rewrite this again, what counts as satisfactory? I feel like we're chasing perfection. It's complicated no matter how you slice it. I'm not trying to be disagreeable and if you say rewrite it I'll go rewrite it, but this feels like the sort of thing where we're both making equally valid judgement calls and where a third party would find both our judgement calls to be equally difficult to grasp without explanation. |
|
I'm really not ok with storing a |
|
@asajeffrey I don't like the The only non-aesthetic argument here is if performance matters, in which case we should profile it. |
|
This might still be able to be made cleaner. I'm going to give it another pass in the morning. |
|
Thanks! |
|
I just pushed the cleanup work. If this is good, I'll rebase them together. I don't think it can be made cleaner. Sorry about the turnaround time. Lots of things going on, etc. |
|
Yay, this looks really nice now, thanks for all your work!
|
|
Oh yes, and the commits need squashed too. |
We maintain a stack of open element nodes and non-node elements and use it to determine when to close them.
|
Think that does it. |
|
LGTM! @bors-servo r+ |
|
|
Convert node serialization to a purely iterative algorithm. We maintain a stack of open element nodes with their children count, popping from the top of the stack and closing when the count reaches zero. Contrary to my comment in #16696, this is a purely iterative algorithm. I just wasn't feeling sufficiently clever with respect to finding a relatively clean way until later. <!-- Please describe your changes on the following line: --> --- <!-- 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 #16696 (github issue number if applicable). <!-- Either: --> - [x] There are tests for these changes. <!-- 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/17520) <!-- Reviewable:end -->
|
|
|
@bors-servo: retry |
|
|
|
|
|
Yay! |
camlorn commentedJun 25, 2017
•
edited by larsbergstrom
We maintain a stack of open element nodes with their children count, popping from the top of the stack and closing when the count reaches zero.
Contrary to my comment in #16696, this is a purely iterative algorithm. I just wasn't feeling sufficiently clever with respect to finding a relatively clean way until later.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is