Closed
Bug 1442760
Opened 6 years ago
Closed 6 years ago
Switch nsHTMLTags hashtables to nsDataHashtable
Categories
(Core :: DOM: HTML Parser, enhancement)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
7.75 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
We should really avoid using PL_Hash if possible. Converting `gTagTable` and `gTagAtomTable`[1] is pretty straightforward. [1] https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/parser/htmlparser/nsHTMLTags.h#83-84
Assignee | ||
Comment 1•6 years ago
|
||
This converts nsHTMLTags hashtables from PLHash to nsDataHashtable which gives us both type safety and simpler code. Addtionally `gTagTable` now holds a nsString instead of a raw char16_t pointer, this has the benefit of the strings knowing their sizes allowing for more efficient comparisons. We avoid heap allocations in the nsString by using `AssignLiteral` with the string from the static string array.
Attachment #8956677 -
Flags: review?(hsivonen)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment 2•6 years ago
|
||
Comment on attachment 8956677 [details] [diff] [review] Switch nsHTMLTags hashtables to nsDataHashtable Review of attachment 8956677 [details] [diff] [review]: ----------------------------------------------------------------- ::: parser/htmlparser/nsHTMLTags.cpp @@ +92,5 @@ > if (gTableRefCount++ == 0) { > NS_ASSERTION(!gTagTable && !gTagAtomTable, "pre existing hash!"); > > + gTagTable = new TagStringHash(64); > + gTagAtomTable = new TagAtomHash(64); Is there a reason to use 64 instead of a number derived from the number of tags?
Attachment #8956677 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #2) > Comment on attachment 8956677 [details] [diff] [review] > Switch nsHTMLTags hashtables to nsDataHashtable > > Review of attachment 8956677 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: parser/htmlparser/nsHTMLTags.cpp > @@ +92,5 @@ > > if (gTableRefCount++ == 0) { > > NS_ASSERTION(!gTagTable && !gTagAtomTable, "pre existing hash!"); > > > > + gTagTable = new TagStringHash(64); > > + gTagAtomTable = new TagAtomHash(64); > > Is there a reason to use 64 instead of a number derived from the number of > tags? It's just what we used before, I don't know why it was chosen. For a hashtable we'll want a number a bit larger than the number of entries to avoid collisions so that might be what's going on here.
Pushed by erahm@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8bbb6c0d3214 Switch nsHTMLTags hashtables to nsDataHashtable. r=hsivonen
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8bbb6c0d3214
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•