Bug 14977

Summary:

Hixie's DOM Core performance test shows insert >10x slower than append

Product:

WebKit

Reporter:

Darin Adler <darin>

Component:

DOM

Assignee:

Alexey Proskuryakov <ap>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, oliver

Priority:

Version:

523.x (Safari 3)

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
first cut at a fix	none
first cut at a fix (uploaded wrong the first time)	oliver: review-
proposed fix	mjs: review+

Darin Adler

Reported 2007-08-15 13:38:10 PDT

I noticed that the performance test indicated that insert was 10x slower than append. Then I figured out why. Unnecessary churn in the NodeList code. I have a patch that fixes it, but it could possibly slow down other things so it's not quite ready for review. I didn't make a regression test yet.

Attachments
first cut at a fix (7.35 KB, patch) 2007-08-15 13:44 PDT, Darin Adler	no flags	Details Formatted Diff Diff
first cut at a fix (uploaded wrong the first time) (11.23 KB, patch) 2007-08-15 13:46 PDT, Darin Adler	oliver: review-	Details Formatted Diff Diff
proposed fix (3.81 KB, patch) 2007-11-22 10:16 PST, Alexey Proskuryakov	mjs: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Darin Adler

Comment 1 2007-08-15 13:44:27 PDT

Created attachment 15985 [details] first cut at a fix

Darin Adler

Comment 2 2007-08-15 13:46:27 PDT

Created attachment 15986 [details] first cut at a fix (uploaded wrong the first time)

Oliver Hunt

Comment 3 2007-09-05 22:06:32 PDT

Comment on attachment 15986 [details] first cut at a fix (uploaded wrong the first time) +private: + virtual bool nodeMatches(Node*) const; private virtual? Nothing else leaps out at me as being odd, but i don't feel qualified to comment on risk or correctness :( My ad hoc benchmarking of this show Insertion time (from hixies DOM Core test) go from ~1100ms (avg. of 3 runs) to ~110ms (3 runs)

Oliver Hunt

Comment 4 2007-09-05 23:57:04 PDT

Comment on attachment 15986 [details] first cut at a fix (uploaded wrong the first time) Okay, after repeated runs (mostly cleaning my tree) i don't think this is safe, it's a huge regression for the Indexd portion of Hixies DOM Core tests

Oliver Hunt

Comment 5 2007-09-05 23:58:52 PDT

http://nontroppo.org/timer/Hixie_DOM.html The index test goes from 26ms to >1000ms

Alexey Proskuryakov

Comment 6 2007-11-22 10:16:28 PST

Created attachment 17443 [details] proposed fix This doesn't cause regressions in other tests. I couldn't fully understand the original patch, so I started from scratch. It's possible that I missed some additional optimization opportunities that were present there.

Alexey Proskuryakov

Comment 7 2007-11-22 10:19:08 PST

> Unnecessary churn in the NodeList code. In other words, the problem had nothing to do with insertion performance - it's the previous test (index) that was making us register too many NodeLists, and thus slowed down subsequent DOM mutations.

Maciej Stachowiak

Comment 8 2007-11-22 19:33:44 PST

Comment on attachment 17443 [details] proposed fix r=me

Alexey Proskuryakov

Comment 9 2007-11-22 22:44:20 PST

Committed revision 27983.

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