599982 - Optimize nsHTMLContentSerializer::AppendAndTranslateEntities

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

nsHTMLContentSerializer::AppendAndTranslateEntities shows up pretty
badly in GetInnerHTML profiles.

Comment 1

[Boris Zbarsky [:bzbarsky]]

In particular, called from attribute-serialization code....

Comment 2

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

That actually depends on the case, I think.

Comment 3

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

I think I can cut the time spent in AppendAndTranslateEntities to 
half or so in the cases when we don't actually translate any entities.
Patch coming after some testing.

Comment 4

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

Attachment: WIP

A bit ugly approach to make a fast loop for the innerHTML case.

Comment 5

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

test_bug424359-2.html fails with the patch.

Comment 6

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

Attachment: patch

This is really for GetInnerHTML. Helps especially attribute handling.

Comment 7

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

Comment on attachment 479393 [details] [diff] [review]
patch

Bah, I managed to do something wrong here.

Comment 8

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

Attachment: patch

Better to not call -- on PRUint32 variable which is 0.

Comment 9

[Laurent Jouanneau]

Comment on attachment 479483 [details] [diff] [review]
patch


>+      if (val <= kValNBSP && entityTable[val]) {
>+        // So there is something to replace. Iterate from
>+        // end to the first character which needs replament and
>+        // translate needed entities.
>+        nsAutoString str(aStr);

s/replament/replacement


Ok for me.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This still seems like it could be sped up quite a bit in the case when there are multiple characters in the string that needs to be turned into entities. Once you've found something that needs replacing, why not iterate from then on forwards and insert character by character as you go and replace on the fly?

Comment 11

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

I could profile that case too.
But inserting character by character is not cheap!

Comment 12

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

Attachment: lots of entities -testcase

Comment 13

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

Attachment: character by character

Ok, I overestimated the cost of Append.
(And it is getting faster in another bug.)

Comment 14

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

But actually, I need to test also less worst-case scenario.

Comment 15

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

Comment on attachment 481794 [details] [diff] [review]
character by character

This is not good for not-worst-case test.

Comment 16

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

Attachment: less-entities -tescase

Comment 17

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

Attachment: patch

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 481834 [details] [diff] [review]
patch

Actually, I realized that the way we usually do these types of things is that we first do one run-through and calculate the resulting length. Then we do a single allocation and then finally go through and convert the data. This in order to optimize for few allocations.

That's the strategy used for for example UTF8/16 conversion.

Anyhow, the current patch looks ok to me too.

Comment 19

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

thanks for the review.

afaics iterating twice would cause a significant perf regression
in cases when there are no entities.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

With this patch you already are. First iterating while looking for an entity, then iterating when doing the string copy.

Comment 21

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

what "iterating when doing string copy"? Well, ok, if you think of memcpy
as iteration.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, for the simple case when there are no entities it would reduce to the same code being executed. Something like:

Iterate through the string to measure the size. If the resulting size is the length of the string just do an assign. Otherwise call SetCapacity and iterate and either write a single char or write an entity. Once the remaining size is equal to the number of remaining chars, do a single string-append call.

Alternatively:

Iterate through the string to measure the size and also remember the location of the first found entity. Call SetCapacity. Copy up to the first found entity. Iterate and either write a single char or write an entity. Once the remaining size is equal to the number of remaining chars, do a single string-append call.

Comment 23

[Jonathan Watt [:jwatt]]

Does this need need to be pushed?

Comment 24

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

http://hg.mozilla.org/mozilla-central/rev/5066079367fe