SetDocument runs every time you switch the document

[plusplusben]

If sizzle switches between parsing a dom document and an xml document, it calls SetDocument every time it switches. Do you think it would make sense to cache more than just the current document settings?

I created a quick jsfiddle here http://jsfiddle.net/5j9xxocs/ . If you run a profile and then click on the "ClickMe" button, you can see that Sizzle's SetDocument is pretty expensive and runs twice for every iteration. I work on an application that uses Sizzle and switches between parsing XML and DOM frequently. I made a rudimentary fix on my local version of Sizzle that caches the settings for both document types and was able to pretty massively improve the performance of the app.

Would this be something worth adding to Sizzle? Or is my use case pretty edge?

[timmywil]

I would suspect this is not a common issue, but if we did want to cache all setDocument results, I'd want to limit the number of documents cached (to something like 5). The majority of cases where the document switches could probably be covered by a cache size of 2-5.

[dmethvin]

Are there any risks to caching a document, especially an XML one? I could see there being some downsides if you ended up caching a large XML document that would otherwise be garbage collected. With a single doc cache that's not very likely. If it's truly rare, another solution would be to load a second copy of Sizzle.

[timmywil]

There aren't risks to caching one document. The risk comes when there are many documents.

[zachelrath]

It seems like it would be more effective to have a cache based on "type" of Document, with separate entries only for each unique type of Document actually encountered, since a given browser should handle each type of Document consistently. For instance, If you're rapidly switching between separate XML documents, they're all going to have the same feature supports, but they're different Documents. So if the caching was along the lines of "if isXML, use XML doc support cache, if its a DOM fragment, use DOM fragment support cache, etc...".

With this approach, you don't have to deal with an arbitrary size for the number of Documents to cache, or the garbage collection problem, since actual Documents are not being cached.

Back to the Document-based caching approach, though, another possibility to address the garbage collection problem would be to remove cache entries whenever a given node that is an ownerDocument is removed or appended to another document, at which point that node can no longer be an ownerDocument.

[dmethvin]

It seems like it would be more effective to have a cache based on "type" of Document, with separate entries only for each unique type of Document actually encountered, since a given browser should handle each type of Document consistently.

You'd still have the potential issue of holding onto an XML document that would otherwise be garbage collectable, for example something fetched via AJAX. The last fetched XML doc would never be released until a new one arrived, if ever.

For instance, If you're rapidly switching between separate XML documents, they're all going to have the same feature supports, but they're different Documents. So if the caching was along the lines of "if isXML, use XML doc support cache, if its a DOM fragment, use DOM fragment support cache, etc...".

If there were really only two document implementations (XML and HTML) the setDocument() overhead could be reduced by caching feature detects the first time the document type was encountered. I don't think that's always the case though. Different implementations of XML documents can occur within the same page (for example, MSXML.DOMDocument, XML+XHTML, and XMLDocument).

another possibility to address the garbage collection problem would be to remove cache entries whenever a given node that is an ownerDocument is removed or appended to another document, at which point that node can no longer be an ownerDocument.

How would Sizzle know that happened? Also, someone could parse an XML string, select some nodes, and let the XML root go out of scope. This in particular seems like a hairy problem to tackle for an edge case.

[gibson042]

This in particular seems like a hairy problem to tackle for an edge case.

Exactly. There are lots of good ideas and good intentions here, but in practice it's thorny. Pull requests would be evaluated on size, speed, memory consumption, and client compatibility, but I don't foresee myself taking this on.

[timmywil]

Consensus seems to be patch welcome. We would review a PR, but will not tackle this in the immediate future.

[gibson042]

A thought for when this is picked up: cache setDocument output by documentElement nodeName/namespaceURI/etc. It's likely that implementations only privilege HTML and maybe SVG anyway, so we should be able to easily support cases that aren't absolutely bonkers.