1850645 - Make `AutoTextControlHandlingState` stop using `nsString`

Status: RESOLVED FIXED

(Core :: DOM: Forms, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The profile commented in bug 1845185 comment 5 has a lot of samples at the construction of AutoTextControlHandlingState::mSettingValue. I considered its type to nsString because if given string has refcountable buffer, it can avoid copying the buffer. However, refcountable buffer is currently created only in nsStringBuffer::ToString, and the users are a few. Therefore, we could improve the score with changing it to nsAutoString.

However, I don't think longer value than the auto buffer of nsAutoString is rare. Therefore, the approach could be a bad optimization for specific benchmarks. I found nsContentUtils::PopulateStringFromStringBuffer() which makes the buffer forcibly refcountable. Using this could be better solution.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Surprisingly, it's faster in other tests if making AutoTextControlHandlingState::mSettingValue have refcountable buffer. It means that the further string copy cost is more expensive than allocation at the constructor. Keep investigating what should I do.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Change AutoTextControlHandlingState::mSettingValue from nsString to nsAutoString:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=f73f32dfe422ce63f8a4f7ac3653c14b99c6dd90&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=2d6d6cae6cb8fbce0d483057dde17c39edcfe401&page=1

Make AutoTextControlHandlingState::mSettingValue have refcountable string if nsAutoString needs a heap allocation:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=f9ce5055694492687e86eb604cbb8561b9499c1f&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=2d6d6cae6cb8fbce0d483057dde17c39edcfe401&page=1

Make AutoTextControlHandlingState::mSettingValue always have refcountable string:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=1be673d580ae831a44ebc9e6fe9876f67a697636&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=2d6d6cae6cb8fbce0d483057dde17c39edcfe401&page=1

The first result vs. the last result:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=1be673d580ae831a44ebc9e6fe9876f67a697636&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=f73f32dfe422ce63f8a4f7ac3653c14b99c6dd90&page=1

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1850645 - Make `TextControlState` use `nsAutoString` r=smaug!

Currently, the type of AutoTextControlHandlingState::mSettingValue is
nsString, but it's a stack only class, and if the string buffer is
refcountable, nsTAutoStringN(const substring_type&) avoid copying the
memory anyway.
https://searchfox.org/mozilla-central/rev/fccd8c0ac998c4c624b6c255e621d36f72cabe01/xpcom/string/nsTString.h#295
https://searchfox.org/mozilla-central/rev/fccd8c0ac998c4c624b6c255e621d36f72cabe01/xpcom/string/nsTSubstring.cpp#504,513-514,517

So, it should be nsAutoString in any cases. Therefore, this patch changes the
type and fix similar points in TextControlState.cpp.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1850645 - part 2: Make `AutoTextControlHandlingState` allocate refcountable string buffer if allocation is required r=smaug!

Now, AutoTextControlHandlingState::mSettingValue is nsAutoString. However,
if the value length equals or is longer than 64 characters, it needs to allocate
the storage in the heap unless the given string buffer is refcountable.

So, in any cases, we need to pay an allocation cost (even before creating
AutoTextControlHandlingState). Therefore, once the constructor uses
refcountable string buffer for mSettingValue, the users of
AutoTextControlHandlingState::GetSettingValue() can skip another
allocations for receiving the copy.

Depends on D188682

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, with a lot of testing, I believe that AutoTextControlHandlingState::mSettingValue should be nsAutoString to avoid allocation of the string buffer in most cases.

Additionally, if the setting value requires to allocate the buffer in the heap, it should be refcountable buffer because the mSettingValue may be copied during setting the value and we can skip another allocation with using refcountable buffer.

The final result is:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=0de546d7bb0b9fb1cf7fb239c5f951e627db317a&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=78735d10fbb9b5b62a46d787e4acab2ce4e23cdb&page=1

If we make the buffer always refcountable, we get better performance in a lot of cases, but it makes some benchmark score slower. I guess that it's caused by the allocation cost which is not required if we use nsAutoString.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95ff3c89e929
Make `TextControlState` use `nsAutoString` r=smaug

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/95ff3c89e929