Last Comment Bug 588990 - Remove lazy hashing of names to avoid walking full DOM
: Remove lazy hashing of names to avoid walking full DOM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
Mentors:
Depends on: 680922
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-19 16:17 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2011-08-24 05:53 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (12.03 KB, patch)
2010-09-27 12:01 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Patch to fix (27.11 KB, patch)
2011-04-12 10:57 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bzbarsky: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-19 16:17:45 PDT
We currently don't store elements in the name table until someone has looked up a given name. However this means that once someone looks up a specific name for the first time, we can't check the hash for it, but have to walk the full DOM.

It should be more efficient to simply hash all names as they come in. Patch coming up.
Comment 1 Boris Zbarsky [:bz] 2010-09-15 01:54:10 PDT
Jonas, did this patch ever happen?
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-27 12:01:40 PDT
Created attachment 478826 [details] [diff] [review]
WIP

This has been sitting in my tree for a while and should work. However it needs tests so not requesting review just yet.
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-12 10:57:34 PDT
Created attachment 525441 [details] [diff] [review]
Patch to fix

This'll do it. I found two other bugs while writing these tests:

1. Bug 307415
2. Bug 649415
Comment 4 :Ehsan Akhgari 2011-04-19 10:45:17 PDT
Comment on attachment 525441 [details] [diff] [review]
Patch to fix

r=me on the specialpowers patch, which I'm going to use in my patch in bug 650543.

Also, could you please let extensions/spellcheck/hunspell/tests/suggestiontest/Makefile.orig remain in the tree?  :-)
Comment 5 :Ehsan Akhgari 2011-04-19 10:55:09 PDT
Landed the gc API: http://hg.mozilla.org/mozilla-central/rev/61ab7fa02e26
Comment 6 Boris Zbarsky [:bz] 2011-04-22 23:05:48 PDT
Why is nsHTMLDocument::ResolveName using PutEntry instead of GetEntry?

This patch changes our behavior on this testcase:

  data:text/html,<img name="write"><script>alert(document.write)</script>

but I _think_ it changes towards better compat with other browsers... Please check IE?

Is there a point to populating the name map for non-HTML documents?  If not, is it worth optimizing out the add/remove bits for non-HTML documents (I realize we want them for XHTML, so mIsRegularHTML is the wrong thing to check, but...)
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-22 23:21:26 PDT
(In reply to comment #6)
> Why is nsHTMLDocument::ResolveName using PutEntry instead of GetEntry?

When I made similar code refactoring for the id code I tried changing PutEntry to GetEntry. However that caused a measurable slowdown on the relevant Dromaeo tests. Apparently our hash table is faster when a lookup succeeds. 

> This patch changes our behavior on this testcase:
> 
>   data:text/html,<img name="write"><script>alert(document.write)</script>
> 
> but I _think_ it changes towards better compat with other browsers... Please
> check IE?

I checked with other browsers and no others had the same behavior as us. IIRC this patch aligned us with everyone else, and definitely with IE.

> Is there a point to populating the name map for non-HTML documents?  If not,
> is it worth optimizing out the add/remove bits for non-HTML documents (I
> realize we want them for XHTML, so mIsRegularHTML is the wrong thing to
> check, but...)

We could, I don't feel strongly. It does seem somewhat unlikely that there will be significant amounts of named non-form-control HTML contents in non-HTML documents (note that no form controls show up in the CanHaveName list).
Comment 8 Boris Zbarsky [:bz] 2011-04-22 23:33:42 PDT
> However that caused a measurable slowdown on the relevant Dromaeo tests.

See bug 572614 comment 2 and bug 572614 comment 3.
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-22 23:54:05 PDT
There you go :) I'll switch to GetEntry then :)
Comment 10 Boris Zbarsky [:bz] 2011-04-22 23:56:09 PDT
Comment on attachment 525441 [details] [diff] [review]
Patch to fix

OK, r=me with the change to GetEntry and corresponding comment adjustments.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-04-23 01:37:36 PDT
Looks like you could have removed the NAME_NOT_VALID define?
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-25 09:55:58 PDT
Checked in. Thanks for the review!

Of course, I forgot to remove the part of the patch that killed Makefile.orig, so I had to land a followup which added it back.

http://hg.mozilla.org/mozilla-central/rev/ea254208e04c
http://hg.mozilla.org/mozilla-central/rev/de123d952abd

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