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 upCache document title to avoid unnecessary traversals #6744
Conversation
highfive
commented
Jul 25, 2015
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon. |
hoppipolla-critic-bot
commented
Jul 25, 2015
|
Critic review: https://critic.hoppipolla.co.uk/r/5649 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
|
|
nick-thompson
force-pushed the
nick-thompson:cache_document_title
branch
from
ef6aa39
to
302db3e
| }); | ||
|
|
||
| self.current_title_element.set( | ||
| title.r().and_then(HTMLTitleElementCast::to_ref).map(JS::from_ref)); |
This comment has been minimized.
This comment has been minimized.
nick-thompson
Jul 27, 2015
Author
Contributor
So it turns out this line contains the bug responsible for the two failing subtests in /html/dom/documents/dom-tree-accessors/document.title-09.html I mentioned. In the case of the SVG namespace, the Node returned by searching the child_elements() of root.r() does not pass the check is_htmltitleelement(). This check is used under the hood of HTMLTitleElementCast::to_ref when casting a Node to an HTMLTitleElement, such that the result of the cast, when the argument does not pass is_htmltitleelement() is None. This then breaks the subsequent calls to Title() in the test, because we've cached None when we should have cached an HTMLTitleElement.
@nox I'm not sure what the right solution to this issue is. Should document.current_title_element be of type MutNullableHeap<JS<Node>>, or MutNullableHeap<JS<Element>>, so that we can avoid the explicit cast to HTMLTitleElement? Or maybe is_htmltitleelement() should accommodate title elements of the SVG namespace? Or maybe SVG title elements aren't (and shouldn't be) actual HTMLTitleElements?
This comment has been minimized.
This comment has been minimized.
nox
Jul 31, 2015
Member
AFAICT, there should be SVG elements that aren't deriving from HTMLElement, but that's a bit huge to implement right now just for this. Maybe cache the title only if not in a HTML document for now?
|
@nox going to review this? |
|
|
nick-thompson
force-pushed the
nick-thompson:cache_document_title
branch
from
302db3e
to
01ae65b
|
Hm.. that @nox this update falls back to a traversal on a cache miss in the case of an SVG document, but not in the case of an HTML document, because the cache miss would not have occurred if the document were an HTML document and a title element existed. My implementation feels unnecessarily convoluted though... what do you think? |
|
That looks like a bug in tidy.py, unfortunately. We'll need to solve that for this to merge. |
|
|
|
@nick-thompson Sounds good to me. Do you think you can rebase your branch, or did you become disinterested in this patch (nothing wrong with that)? |
|
Thoughts on moving forward with this? |
|
Ping me so that I reopen it if you work on it again, meanwhile I'm closing it. |
nick-thompson commentedJul 25, 2015
Fairly simple perf enhancement as per issue #6713.
cc @nox. I have two failing subtests in /html/dom/documents/dom-tree-accessors/document.title-09.html that I'm still working out, but otherwise I think this is ready. Wanted to put it up sooner than later to start getting your feedback. Thanks!