Bug Tracker

Opened 9 years ago

Last modified 8 years ago

#6863 closed enhancement

faster getText — at Version 17

Reported by: steida Owned by:
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 (17)

comment:1 Changed 9 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 9 years ago by steida

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

comment:3 Changed 9 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 9 years ago by SlexAxton

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

comment:5 Changed 9 years ago by paul.irish

Should definitely be using elem.textContent if supported.

comment:6 in reply to:  5 Changed 9 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 9 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 9 years ago by wookiehangover

Owner: set to wookiehangover
Status: openassigned

I'd like to take a crack at this!

comment:9 Changed 9 years ago by wookiehangover

Owner: wookiehangover deleted
Status: assignedopen

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

comment:10 Changed 9 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 9 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 9 years ago by john

Keywords: 1.7-discuss added

Nominating ticket for 1.7 discussion.

comment:13 Changed 9 years ago by Rick Waldron

Description: modified (diff)

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

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

comment:14 Changed 9 years ago by jaubourg

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

comment:15 Changed 9 years ago by ajpiano

Description: modified (diff)

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

comment:16 Changed 9 years ago by timmywil

Description: modified (diff)

+0, skeptical on the performance gain.

comment:17 Changed 9 years ago by dmethvin

Description: modified (diff)

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

Note: See TracTickets for help on using tickets.