#6863 closed enhancement (fixed)
faster getText
Reported by: | steida | Owned by: | Timmy Willison |
---|---|---|---|
Priority: | blocker | Milestone: | 1.7 |
Component: | selector | Version: | 1.4.2 |
Keywords: | speed, getText, 1.7-discuss | Cc: | |
Blocked by: | Blocking: |
Description (last modified by )
var getText = function (elem) {
var text = []; for (var node = elem.firstChild; node; node = node.nextSibling) {
Get the text from text nodes and CDATA nodes
if (node.nodeType == 3 node.nodeType == 4) text.push(node.nodeValue);
Traverse everything else, except comment nodes else if (elem.nodeType !== 8)
text.push(getText(node));
} return text.join();
};
Change History (25)
comment:1 Changed 13 years ago by
Component: | unfiled → selector |
---|
comment:2 Changed 13 years ago by
Also DOM traversing by nextSibling is almost twice faster then index based.
comment:3 Changed 13 years ago by
This new implementation suffers from the same inconsistency as the current version, reported in ticket:6827. A corrected version would be
var getText = function (elem) { var text = []; for (var node = elem.firstChild; node; node = node.nextSibling) { // Get the text from text nodes and CDATA nodes if (node.nodeType == 3 || node.nodeType == 4) text.push(node.nodeValue); // Special case for script nodes else if (node.tagName === 'SCRIPT') text.push(node.text); // Traverse everything else, except comment nodes else if (elem.nodeType !== 8) text.push(getText(node)); } return text.join(); };
comment:4 Changed 13 years ago by
Keywords: | speed getText added |
---|---|
Milestone: | 1.4.3 → 1.5 |
Priority: | → low |
Status: | new → open |
comment:5 follow-up: 6 Changed 13 years ago by
Should definitely be using elem.textContent
if supported.
comment:6 Changed 12 years ago by
Replying to paul.irish:
Should definitely be using
elem.textContent
if supported.
We had problems getting a consistent value back from the built-in string methods. The only way to get consistent results across all browsers was to do the traversal.
Naturally, a patch to the contrary would be welcome.
comment:7 Changed 12 years ago by
Milestone: | → 1.next |
---|
This is a relatively simple change (using arrays instead of concatenation). Should be low-hanging fruit for someone - possibly in 1.7.
comment:8 Changed 12 years ago by
Owner: | set to wookiehangover |
---|---|
Status: | open → assigned |
I'd like to take a crack at this!
comment:9 Changed 12 years ago by
Owner: | wookiehangover deleted |
---|---|
Status: | assigned → open |
didn't realize that this was related to sizzle... bailing out
comment:10 Changed 12 years ago by
I created a pull request that fixes this issue at http://github.com/jquery/sizzle/pull/59.
As noted in the pull request, this change may have a negative impact on performance in Chrome, Firefox 4 and IE8+. It will have a minor performance benefit in IE6 and IE7, Safari (as of version 5.0.5) and Opera (as of 11.10).
comment:11 Changed 12 years ago by
https://github.com/jquery/sizzle/pull/59
I updated the pull request based on feedback from dmethvin and created a jsPerf that measures the performance improvement: http://jsperf.com/sizzle-gettext-bugfix-6863.
comment:12 Changed 12 years ago by
Keywords: | 1.7-discuss added |
---|
Nominating ticket for 1.7 discussion.
comment:13 Changed 12 years ago by
Description: | modified (diff) |
---|
+1, Changes actually go to Sizzle I believe - if it can be reasonably achieved
comment:15 Changed 12 years ago by
Description: | modified (diff) |
---|
+1, Yes, but also +1 on jaubourg's sentiment above !
comment:16 Changed 12 years ago by
Description: | modified (diff) |
---|
+0, skeptical on the performance gain.
comment:17 Changed 12 years ago by
Description: | modified (diff) |
---|
+1, Worth a look with #3144 as well.
comment:18 Changed 12 years ago by
See also this forum post with a link to perf tests there and also linked above:
http://forum.jquery.com/topic/sizzle-gettext-open-pull-request-for-ticket-6863#14737000002377958
Would still like to see #3144 tackled at the same time if possible to reduce some of the quirks of IE6/7/8 text behavior using the code suggested by ioquatix and linked in the fiddle at the bottom. It might make the function even faster since it eliminates recursion for all but XML documents.
comment:19 Changed 12 years ago by
Description: | modified (diff) |
---|
+1, Well, certainly seems to be running faster. If it's passing, let's get it in.
comment:22 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Milestone: | 1.next → 1.7 |
Priority: | low → blocker |
comment:23 Changed 12 years ago by
Owner: | set to dmethvin |
---|---|
Status: | open → assigned |
comment:24 Changed 12 years ago by
Owner: | changed from dmethvin to Timmy Willison |
---|
Thanks for taking this timmywil!
TL;DR: There is a pull already for this in Sizzle's tracker: http://bugs.jquery.com/ticket/6863#comment:10
Ticket #3144 has an alternate implementation using .textContent/.innerText when possible that may be even faster, and it addresses issues with IE6/7/8 losing spaces.
comment:25 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Update Sizzle. Fixes #3144, #6863.
Changeset: 22fcc7744daded0dc0783d85df3bd88c6dbc4544
This wouldn't make much of a difference unless there were a lot of strings being concatenated, which is probably not the most common case. However, it seems like pretty low hanging fruit and I know it makes a big difference in IE6/7.