1807017 - Consider to reuse the memory when nsHtml5Tokenizer::end() is called

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

https://searchfox.org/mozilla-central/rev/e6b709df9b93858364f02ab89f40d78762693db8/parser/html/nsHtml5Tokenizer.cpp#4998 shows up a bit in some speedometer 2 profiles, at least in VanillaJS. We seem to be deleting the autoJArray.arr. Could you reuse that memory somehow?

Comment 1

[Henri Sivonen (:hsivonen)]

This is for innerHTML/DOMParser and not for parse from the network, right? And specifically for nsHtml5Tokenizer::strBuf, right?

For innerHTML/DOMParser it would be feasible to reuse that memory. The risk is that strBuf stretches to the worst case of the parser invocation, so if we unconditionally reuse the memory, a parse that needs an exceptionally long buffer would cause a memory leak-like situation with the long buffer sticking around for the rest of the process lifetime.

So we'd probably need some mechanism to delete an overlong buffer anyway. If we just put a magic threshold in nsHtml5Tokenizer::end, the delete would still show up on profiles if benchmark caused allocation above the threshold in a loop.

The fancy way would probably be to have an idle task delete the buffer if it's longer than some magic constant.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

This is for innerHTML, yes.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

This is starting to show up quite a lot in some cases.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Could we do something simple now and keep the buffer around say whenever it is <= 1024 ?
It is though often quite a lot larger, up to 65536. Clearing using an idle runnable might be better.

I'd need to re-learn how to add some very Gecko specific stuff into .java, so perhaps you could write a small patch?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I guess I can do this on nsHtml5StringParser level, no need to touch java side at all.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1807017 - Consider to reuse the memory when nsHtml5Tokenizer::end() is called, r=hsivonen

Hopefully this isn't too complicated, but I wanted to make sure the runnable doesn't keep any other objects alive and thus
it needed to have Disconnect to clear the raw pointer.
The basic idea is to cache the previous buffer until we have idle time.
This doesn't seem to affect performance enough to show up in numbers, but based on the profiles this does
affect execution by reducing the overhead coming from nsHtml5Tokenizer::end()

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1807017 part 1 - Allow HTML parser to retain internal buffers.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a317563d8729
Consider to reuse the memory when nsHtml5Tokenizer::end() is called, r=hsivonen

Comment 9

[Cristina Horotan [:chorotan]]

Backed out changeset a317563d8729 (bug 1807017) for causing mutiple failures at nsHtml5TreeBuilder.cpp

Backout: https://hg.mozilla.org/integration/autoland/rev/495c79b4cbf4bf8847f51fd572b03c4b5f0a4835

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=a317563d872992a27d1417d2edc2e086fdc4b731

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=452592406&repo=autoland&lineNumber=2029
https://treeherder.mozilla.org/logviewer?job_id=452583736&repo=autoland&lineNumber=89848

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ce47a20dd08
Consider to reuse the memory when nsHtml5Tokenizer::end() is called, r=hsivonen

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/2ce47a20dd08

Comment 12

[Henri Sivonen (:hsivonen)]

https://github.com/validator/htmlparser/commit/1297623a20b4b6bc1986282f7ce16f82b6ae8a53