Bug Tracker

Modify

Ticket #6782 (closed enhancement: fixed)

Opened 4 years ago

Last modified 2 years ago

carefully allow more strings to use innerHTML

Reported by: cmcnulty Owned by: dmethvin
Priority: blocker Milestone: 1.7
Component: manipulation Version: 1.4.4
Keywords: html,1.7-discuss Cc:
Blocking: Blocked by:

Description (last modified by dmethvin) (diff)

.html() uses rnocache to determine both if a snippet should be cached by buildFragment() and also whether or not html() should use append or innerHTMLto insert the snippet. The problem is that buildFragment requires more stringent rules than innerHTML does, and therefore fewer strings use innerHTML than otherwise could, and that hurts performance, particularly by stripping all snippets with an <option anywhere in the string.

I therefore suggest a new regex rule explicitly to determine if innerHTML or append method should be used. Not only will the improve performance, but it also makes the code cleaner, more readable and make more sense.

The new regex only excludes <style and <script for the following documented reasons:

style:  http://dev.jquery.com/ticket/5977

script:  http://poeticcode.wordpress.com/2007/10/03/innerhtml-and-script-tags/

<object and <embed

Have tests added to them, and cause no new failures in IE7, FF3.7 or Chrome 5.

I discussed these changes a while back in this topic:

 http://forum.jquery.com/topic/html-performance-vs-cruft#14737000000784382

Attachments

html_innerhtml_patch.txt Download (1.2 KB) - added by cmcnulty 4 years ago.
manipulation-innerHTML-tests.txt Download (1.4 KB) - added by cmcnulty 4 years ago.

Change History

Changed 4 years ago by cmcnulty

Changed 4 years ago by cmcnulty

comment:1 Changed 3 years ago by snover

  • Milestone 1.4.3 deleted

Resetting milestone to future.

comment:2 Changed 3 years ago by snover

  • Priority set to high
  • Status changed from new to open
  • Version changed from 1.4.2 to 1.4.4
  • Milestone set to 1.5

comment:3 Changed 3 years ago by cmcnulty

Created a jsperf test to show the performance increase:

 http://jsperf.com/tweaks-to-rnocache

I extend jquery to create a .html2() which is identical to .html() except that rather than using nocache it uses a regex test that it optimized for .html()

comment:4 Changed 3 years ago by cmcnulty

comment:5 Changed 3 years ago by john

  • Milestone set to 1.next

comment:6 Changed 3 years ago by john

#7156 is a duplicate of this ticket.

comment:7 Changed 3 years ago by john

#7338 is a duplicate of this ticket.

comment:8 Changed 3 years ago by wookiehangover

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

I'm gonna take crack at this.

comment:9 Changed 3 years ago by cmcnulty

Glad to hear this has been assigned. I've been pushing for this change for a long time and am very familiar with at least some of the issues involved. Feel free to let me know if you have any questions! I'm not convinced that this little change is going to be a cure-all for the #7341 problems, but it should help a bit in IE 7,8,9.

comment:10 Changed 3 years ago by john

  • Keywords html,1.7-discuss added; html removed

Nominating ticket for 1.7 discussion.

comment:11 follow-up: ↓ 14 Changed 3 years ago by jaubourg

  • Description modified (diff)

+0, What about IE6? Need proof.

comment:12 Changed 3 years ago by ajpiano

  • Description modified (diff)

+1, If the perf here is demonstrable in IE, then let's go for it.

comment:13 Changed 3 years ago by timmywil

  • Description modified (diff)

+1, I'm all for perf improvements.

comment:14 in reply to: ↑ 11 Changed 3 years ago by cmcnulty

Replying to jaubourg:

+0, What about IE6? Need proof.

IE6 sees near 100% performance increase, passes all unit tests.

comment:15 follow-up: ↓ 16 Changed 3 years ago by rwaldron

  • Description modified (diff)

@cmcnulty can you provide a test case that we can view in IE6?

comment:16 in reply to: ↑ 15 Changed 3 years ago by cmcnulty

Replying to rwaldron:

@cmcnulty can you provide a test case that we can view in IE6?

I'd be happy to, but I'm not sure exactly what you're looking for. I've tested the change in  jsperf with IE6, and the  pull request passes all of the unit tests. The nice thing about this change is that existing unit tests already cover pretty much this entire change except for <EMBED which is the only thing the pull request adds a unit test for. (None currently exist for EMBED anywhere in the unit tests) Other than that, there are already tests in place for, for instance, making sure that an option stays selected, making sure the <OBJECT is inserted properly, etc.

comment:17 Changed 3 years ago by dmethvin

+1

comment:18 Changed 3 years ago by john

  • Description modified (diff)

+1, If we think that this might work as we expect it to, then let's go for it.

comment:19 Changed 3 years ago by scott.gonzalez

+1

comment:20 Changed 3 years ago by addyosmani

+1

comment:22 Changed 3 years ago by rwaldron

+1

comment:23 Changed 3 years ago by jzaefferer

+0

comment:24 Changed 3 years ago by dmethvin

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

comment:25 Changed 3 years ago by john

  • Owner changed from wookiehangover to dmethvin

comment:26 Changed 3 years ago by dmethvin

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

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.