24517 – REGRESSION (r41552): innerHTML does an updateLayout -- unneeded and can be slow

Bug 24517 - REGRESSION (r41552): innerHTML does an updateLayout -- unneeded and can be slow

Summary: REGRESSION (r41552): innerHTML does an updateLayout -- unneeded and can be slow

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	DOM (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Mac OS X 10.5

Importance:	P2 Normal
Assignee:	Darin Adler

URL:
Keywords:	Regression

Depends on:
Blocks:

Reported:	2009-03-11 09:58 PDT by Darin Adler
Modified:	2009-03-17 16:35 PDT (History)
CC List:	1 user (show)

See Also:

Attachments
work in progress -- missing ChangeLog but otherwise may be ready to go (6.67 KB, patch) 2009-03-11 09:59 PDT, Darin Adler	no flags	Details \| Formatted Diff \| Diff
patch (9.40 KB, patch) 2009-03-11 17:46 PDT, Darin Adler	no flags	Details \| Formatted Diff \| Diff
improved patch (9.70 KB, patch) 2009-03-13 06:49 PDT, Darin Adler	hyatt: review+	Details \| Formatted Diff \| Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Darin Adler 2009-03-11 09:58:46 PDT

I caused this regression recently and Hyatt and I discussed a fix.

Comment 1 Darin Adler 2009-03-11 09:59:25 PDT

Created attachment 28482 [details]
work in progress -- missing ChangeLog but otherwise may be ready to go

Comment 2 Darin Fisher (:fishd, Google) 2009-03-11 15:51:52 PDT

Just an FYI:  On one of the Chrome performance tests, I found this to be about a 3-4% regression.  That test has about 700 assignments to innerHTML over the course of 56 pages.

Comment 3 Darin Adler 2009-03-11 17:46:08 PDT

Created attachment 28507 [details]
patch

Comment 4 Darin Adler 2009-03-11 20:25:27 PDT

Given their interest in this bug, I'm hoping that Dave Hyatt or Darin Fisher will review the patch.

This is the fix I discussed with Hyatt.

Comment 5 Darin Adler 2009-03-11 20:25:55 PDT

I could make the AppendMarkupState thing a bit more elegant by making that "state" object into a class, with the appendMarkup function a member function.

Comment 6 Darin Adler 2009-03-13 06:49:58 PDT

Created attachment 28578 [details]
improved patch

Comment 7 Dave Hyatt 2009-03-17 16:33:44 PDT

Comment on attachment 28578 [details]
improved patch

r=me

Comment 8 Darin Adler 2009-03-17 16:35:33 PDT

http://trac.webkit.org/changeset/41791