Closed Bug 1355479 Opened 4 years ago Closed 4 years ago

nsHtml5HtmlAttributes::nsHtml5HtmlAttributes is malloc heavy

Categories

(Core :: DOM: HTML Parser, defect, P1)

Product:
Core
Component:
DOM: HTML Parser
Version:
50 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: smaug, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1355479 - Flatten attribute storage in the HTML parser to AutoTArray to avoid malloc.
59 bytes, text/x-review-board-request
wchen
: review+
Details
Java changes
74.90 KB, patch
wchen
: review+
Details | Diff | Splinter Review 
No description provided.
Priority: -- → P1
Blocks: 1352978
I'm now blocked on this, so pursuing this myself after all. wchen, please let me know if you already started on this.
Assignee: nobody → hsivonen
No, you can go ahead and fix it.
(In reply to William Chen [:wchen] from comment #2)
> No, you can go ahead and fix it.

OK. Now I regret I didn't push for the isindex removal earlier, since the attribute holder has isindex-specific methods.
I removed isindex support from the Java side in order to eliminate a case where the distinction of the C++ side no longer holding nsHtml5AttributeName objects in the attribute holder would have caused a conflict between C++ and Java.

Isindex removal (bug 1266495) on the C++ is blocked on telemetry (bug 1356181).

In general, this patch ended up affecting more places in the code than I had anticipated.
Attachment #8859091 - Flags: review?(wchen)
Attached patch Java changes — — Splinter Review
Attachment #8859591 - Flags: review?(wchen)
Comment on attachment 8859591 [details] [diff] [review]
Java changes

Review of attachment 8859591 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but is there a reason why we don't want to also reuse non-interned AttributeName objects in Java as well? It seems like it would be better if the code didn't diverge so that we could also catch and reproduce related bugs in Java.
Attachment #8859591 - Flags: review?(wchen) → review+
Comment on attachment 8859091 [details]
Bug 1355479 - Flatten attribute storage in the HTML parser to AutoTArray to avoid malloc.

https://reviewboard.mozilla.org/r/131116/#review137998
Attachment #8859091 - Flags: review?(wchen) → review+
(In reply to William Chen [:wchen] from comment #10)
> Comment on attachment 8859591 [details] [diff] [review]
> Java changes
> 
> Review of attachment 8859591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, but is there a reason why we don't want to also reuse
> non-interned AttributeName objects in Java as well? It seems like it would
> be better if the code didn't diverge so that we could also catch and
> reproduce related bugs in Java.

Taking a quick look, it seems like if we change addAttribute in HtmlAttributes.java to create a new AttributeName instance and call setNameForNonInterned for non-interned names, we would be able to use the same code path for Java and C++ for most of the patch (maybe the isindex divergence would be resolved as well). If it's more complicated than that, perhaps it's not worth the effort.
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5fc028d97c75
Flatten attribute storage in the HTML parser to AutoTArray to avoid malloc. r=wchen
https://hg.mozilla.org/mozilla-central/rev/5fc028d97c75
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
https://hg.mozilla.org/projects/htmlparser/rev/03fa6735cfa8bdeaafede627e9aacdbce4dee8ab
Mozilla bug 1355479 - Remove isindex on the Java side and allow the C++ side to reduce malloc in attribute handling. r=wchen.
(In reply to William Chen [:wchen] from comment #12)
> (In reply to William Chen [:wchen] from comment #10)
> > Comment on attachment 8859591 [details] [diff] [review]
> > Java changes
> > 
> > Review of attachment 8859591 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me,

Thanks.

> > but is there a reason why we don't want to also reuse
> > non-interned AttributeName objects in Java as well? It seems like it would
> > be better if the code didn't diverge so that we could also catch and
> > reproduce related bugs in Java.
> 
> Taking a quick look, it seems like if we change addAttribute in
> HtmlAttributes.java to create a new AttributeName instance and call
> setNameForNonInterned for non-interned names, we would be able to use the
> same code path for Java and C++ for most of the patch (maybe the isindex
> divergence would be resolved as well). If it's more complicated than that,
> perhaps it's not worth the effort.

If we reused the non-interned AttributeName, we'd have to create something else for storing the fields of AttributeName within HtmlAttributes. In Java, that would mean a separate Java array for each field of AttributeName. Especially when Java is used in a mode where multiple attribute holders are created during the parse (e.g. buffered SAX), that would potentially mean more allocations than currently.

As for setNameForNonInterned vs. constructor in Java, using the constructor allows the fields to be final in Java (in case HotSpot cares).

The isindex divergence is going away once we remove isindex from C++. (I'll do that once we've seen release-channel telemetry.)
You need to log in before you can comment on or make changes to this bug.