Closed
Bug 413629
Opened 17 years ago
Closed 3 years ago
Investigate if parser could be bypassed in some cases when setting .innerHTML
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 922681
People
(Reporter: smaug, Assigned: bent.mozilla)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(2 files)
3.56 KB,
patch
|
hsivonen
:
feedback+
|
Details | Diff | Splinter Review |
1.52 KB,
text/html
|
Details |
See the "wrong" patch for bug 404386, (attachment 297391 [details] [diff] [review]) and https://bugzilla.mozilla.org/show_bug.cgi?id=404386&mark=4,6,7#c4
Reporter | ||
Comment 1•17 years ago
|
||
mrbkap, is there some easy way to know if some element needs special handling, or other way round, if the element doesn't need special handling?
Assignee | ||
Comment 2•17 years ago
|
||
Yeah, this is on my list of things to tackle as son as bug 404386 is fixed. I'll probably start on it today.
Assignee: nobody → bent.mozilla
Flags: blocking1.9?
Assignee | ||
Comment 3•17 years ago
|
||
bz, I have a patch ready for this, all I'm waiting on is figuring out which elements we want to optimize for. Jst and I looked through the parser code and it looks like we could trivially expose some "tag can accept text" method there, but it looks like there are all sorts of silly elements that would get included that way (like <bgsound>, for instance). Do you think it's worth leaving it up to the parser or should we just hardcode some small-ish (10?) list of common elements?
Comment 4•17 years ago
|
||
Is this really a big enough win that: 1) We want the added complexity (esp. as the HTML5 spec changes) 2) We want the added cycles in the cases when non-text stuff is being set (how much of a hit is this in that case?) For the rest, I can't answer that question as posed. To answer it we'd need some in-the-wild data that indicates what the frequency of the various cases is.
Comment 5•17 years ago
|
||
I'm not sure it is at this stage of the release. I'm all for generic optimizations and this is something we can certainly improve later. But it is not clear we should spend time on this right now.
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Reporter | ||
Comment 6•14 years ago
|
||
I think we should think about this again. Some perf tests do test this; setting a short value using .innerHTML. Henri, is there some "easy" way to detect whether a string needs to be parsed or whether we could just use nsContentUtils::SetNodeTextContent or something similar?
Comment 7•14 years ago
|
||
(In reply to comment #6) > Henri, is there some "easy" way to detect whether a string needs to be > parsed or whether we could just use nsContentUtils::SetNodeTextContent > or something similar? I think you could just set text if: * The string doesn't contain <, & or U+0000 AND * The context node is not one of: html, head, noscript, table, thead, tfoot, tbody, tr, colgroup, frameset, frame, pre, listing or textarea. In the case of pre, listing and textarea if the string doesn't contain <, & or U+0000, you could delete a leading line feed (if any) and then set the text content.
Reporter | ||
Comment 8•14 years ago
|
||
Could we do something like this. Basically opt-in for the commonly used elements. The patch tries to not regress too badly setting markup in a loop.
Reporter | ||
Comment 9•14 years ago
|
||
This is a bit modified testcase from a webkit bug. (HTML5 parser regressed innerHTML quite a bit there.)
Reporter | ||
Updated•14 years ago
|
Attachment #490864 -
Attachment is patch: false
Attachment #490864 -
Attachment mime type: text/plain → text/html
Reporter | ||
Comment 10•14 years ago
|
||
The patch gives over 50% speed up in that testcase.
Reporter | ||
Comment 11•14 years ago
|
||
Comment on attachment 490863 [details] [diff] [review] WIP Henri, what do you think about something like this?
Attachment #490863 -
Flags: feedback?(hsivonen)
Comment 12•14 years ago
|
||
Comment on attachment 490863 [details] [diff] [review] WIP This looks OK to me, but I'm a bit uncomfortable about whitelisting 3 elements instead of blacklisting the weird ones. (I realize that the blacklist would be longer and would either involve more pointer compares or a virtual call.)
Attachment #490863 -
Flags: feedback?(hsivonen) → feedback+
Comment 13•14 years ago
|
||
If we're willing to make nodeinfo structs a bit bigger, we could add a flags field to nodeinfo that would store info about the node type.... In fact this might be worth doing and would allow us to optimize a whole bunch of stuff. Separate bug? Or heck, we could use an nsINode flag if we want to make this really fast...
Reporter | ||
Comment 14•14 years ago
|
||
We'd need two flags. "Is it ok to bypass parser" and "should delete leading line feed". But I guess we have plenty of bits left atm in nsINode.
Comment 15•14 years ago
|
||
Right. We should do a followup on reorganizing bits; need to think about whether they should all live on Node, or some on Element or what, and whether some should be on nodeinfo or not (more expensive to get those, since it needs an extra fetch).
Comment 16•10 years ago
|
||
Firefox is a lot faster than competition on the attached testcase. Nightly - avg 200.35 stdev 5.668112560632508 Chrome 32 - avg 377.7 stdev 3.451086785347479 IE 11 - avg 1082.85 stdev 19.817353506459937
Reporter | ||
Comment 17•10 years ago
|
||
I'd say this was fixed in bug 922681.
Updated•3 years ago
|
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•