#6863 closed enhancement (fixed)
faster getText
| Reported by: | steida | Owned by: | timmywil |
|---|---|---|---|
| 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 9 years ago by
| Component: | unfiled → selector |
|---|
comment:2 Changed 9 years ago by
Also DOM traversing by nextSibling is almost twice faster then index based.
comment:3 Changed 9 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 9 years ago by
| Keywords: | speed getText added |
|---|---|
| Milestone: | 1.4.3 → 1.5 |
| Priority: | → low |
| Status: | new → open |
comment:5 follow-up: 6 Changed 9 years ago by
Should definitely be using elem.textContent if supported.
comment:6 Changed 8 years ago by
Replying to paul.irish:
Should definitely be using
elem.textContentif 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 8 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 8 years ago by
| Owner: | set to wookiehangover |
|---|---|
| Status: | open → assigned |
I'd like to take a crack at this!
comment:9 Changed 8 years ago by
| Owner: | wookiehangover deleted |
|---|---|
| Status: | assigned → open |
didn't realize that this was related to sizzle... bailing out
comment:10 Changed 8 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 8 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:13 Changed 8 years ago by
| Description: | modified (diff) |
|---|
+1, Changes actually go to Sizzle I believe - if it can be reasonably achieved
comment:15 Changed 8 years ago by
| Description: | modified (diff) |
|---|
+1, Yes, but also +1 on jaubourg's sentiment above !
comment:16 Changed 8 years ago by
| Description: | modified (diff) |
|---|
+0, skeptical on the performance gain.
comment:18 Changed 8 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 8 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 8 years ago by
| Description: | modified (diff) |
|---|---|
| Milestone: | 1.next → 1.7 |
| Priority: | low → blocker |
comment:23 Changed 8 years ago by
| Owner: | set to dmethvin |
|---|---|
| Status: | open → assigned |
comment:24 Changed 8 years ago by
| Owner: | changed from dmethvin to timmywil |
|---|
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 8 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.