Bug Tracker

Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#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 dmethvin)

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 dmethvin

Component: unfiledselector

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.

comment:2 Changed 13 years ago by steida

Also DOM traversing by nextSibling is almost twice faster then index based.

comment:3 Changed 13 years ago by WouterTinus

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 SlexAxton

Keywords: speed getText added
Milestone: 1.4.31.5
Priority: low
Status: newopen

comment:5 Changed 13 years ago by paul.irish

Should definitely be using elem.textContent if supported.

comment:6 in reply to:  5 Changed 12 years ago by john

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 john

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 wookiehangover

Owner: set to wookiehangover
Status: openassigned

I'd like to take a crack at this!

comment:9 Changed 12 years ago by wookiehangover

Owner: wookiehangover deleted
Status: assignedopen

didn't realize that this was related to sizzle... bailing out

comment:10 Changed 12 years ago by barberboy

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).

http://jsperf.com/ultimate-string-concatenation-tests.

comment:11 Changed 12 years ago by barberboy

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 john

Keywords: 1.7-discuss added

Nominating ticket for 1.7 discussion.

comment:13 Changed 12 years ago by Rick Waldron

Description: modified (diff)

+1, Changes actually go to Sizzle I believe - if it can be reasonably achieved

Last edited 12 years ago by Rick Waldron (previous) (diff)

comment:14 Changed 12 years ago by jaubourg

+1, If performance gain is proven... and only if it proven.

comment:15 Changed 12 years ago by ajpiano

Description: modified (diff)

+1, Yes, but also +1 on jaubourg's sentiment above !

comment:16 Changed 12 years ago by Timmy Willison

Description: modified (diff)

+0, skeptical on the performance gain.

comment:17 Changed 12 years ago by dmethvin

Description: modified (diff)

+1, Worth a look with #3144 as well.

comment:18 Changed 12 years ago by dmethvin

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 john

Description: modified (diff)

+1, Well, certainly seems to be running faster. If it's passing, let's get it in.

comment:20 Changed 12 years ago by scottgonzalez

+1, perf test shows improvements across all browsers

comment:21 Changed 12 years ago by jzaefferer

+1

comment:22 Changed 12 years ago by dmethvin

Description: modified (diff)
Milestone: 1.next1.7
Priority: lowblocker

comment:23 Changed 12 years ago by dmethvin

Owner: set to dmethvin
Status: openassigned

comment:24 Changed 12 years ago by dmethvin

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 Timmy Willison

Resolution: fixed
Status: assignedclosed

Update Sizzle. Fixes #3144, #6863.

Changeset: 22fcc7744daded0dc0783d85df3bd88c6dbc4544

Note: See TracTickets for help on using tickets.