Bug Tracker

Modify

Ticket #6863 (closed enhancement: fixed)

Opened 4 years ago

Last modified 2 years ago

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:
Blocking: Blocked by:

Description (last modified by dmethvin) (diff)

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

comment:1 Changed 4 years ago by dmethvin

  • Component changed from unfiled to selector

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 4 years ago by steida

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

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

  • Keywords speed getText added
  • Priority set to low
  • Status changed from new to open
  • Milestone changed from 1.4.3 to 1.5

comment:5 follow-up: ↓ 6 Changed 3 years ago by paul.irish

Should definitely be using elem.textContent if supported.

comment:6 in reply to: ↑ 5 Changed 3 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 3 years ago by john

  • Milestone set to 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 3 years ago by wookiehangover

  • Owner set to wookiehangover
  • Status changed from open to assigned

I'd like to take a crack at this!

comment:9 Changed 3 years ago by wookiehangover

  • Owner wookiehangover deleted
  • Status changed from assigned to open

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

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

  • Keywords speed,getText,1.7-discuss added; speed getText removed

Nominating ticket for 1.7 discussion.

comment:13 Changed 3 years ago by rwaldron

  • Description modified (diff)

+1, Changes actually go to Sizzle I believe

Version 0, edited 3 years ago by rwaldron (next)

comment:14 Changed 3 years ago by jaubourg

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

comment:15 Changed 3 years ago by ajpiano

  • Description modified (diff)

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

comment:16 Changed 3 years ago by timmywil

  • Description modified (diff)

+0, skeptical on the performance gain.

comment:17 Changed 3 years ago by dmethvin

  • Description modified (diff)

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

comment:18 Changed 3 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 3 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 3 years ago by scott.gonzalez

+1, perf test shows improvements across all browsers

comment:21 Changed 3 years ago by jzaefferer

+1

comment:22 Changed 3 years ago by dmethvin

  • Priority changed from low to blocker
  • Description modified (diff)
  • Milestone changed from 1.next to 1.7

comment:23 Changed 3 years ago by dmethvin

  • Owner set to dmethvin
  • Status changed from open to assigned

comment:24 Changed 3 years ago by dmethvin

  • 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 3 years ago by timmywil

  • Status changed from assigned to closed
  • Resolution set to fixed

Update Sizzle. Fixes #3144, #6863.

Changeset: 22fcc7744daded0dc0783d85df3bd88c6dbc4544

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.