Closed Bug 1393232 Opened 7 years ago Closed 7 years ago

Consider to use nsStringBuffer for nsTextFragment::m2b

Categories

(Core :: DOM: Core & HTML, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

If we used nsStringBuffer, getting the data should be a lot faster than the current memcpy. It would become basically just a simple AddRef.
Blocks: 1346723
Blocks: 1386381
Does nsTextFragment::SetTo() now show up significantly in the profiles of bug 1346723?
If it's regressed, bug 1392181 may be the cause.
I filed this mostly because getting the data is too slow, and it has been slow before any recent changes.
But SetTo could become faster too if we just pass around a string backed by a string buffer.
Priority: -- → P1
I'm at least investigating this.
Assignee: nobody → bugs
After this also SetTo could use string buffer in some cases, but looks like it happens relatively rarely when we pass a string with a string buffer to the relevant APIs.

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/8995a601a95dbb10ebccb09514dc950625d10609
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=8995a601a95dbb10ebccb09514dc950625d10609
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=8995a601a95dbb10ebccb09514dc950625d10609
This seems to help with GetData() since we can just share data.
When running the test 2 from bug 1346723 the time spent under nsGenericDOMDataNode::GetData drops ~66%.
This doesn't affect to SetData and such yet, since we in general don't seem to
pass StringBuffer backed strings to the methods. 
But I think we could use bug 1386381 to tweak that. Perhaps use nsString somewhere and not nsAutoString to avoid some extra memcpying.

Based on an assertion I got in an earlier version of the patch, this might also help with location bar a bit, since we seem to clone 2b textnodes there quite a bit, and with this patch such clone is just one addref when it comes to text data.


This also fixes "REPLACEMENT CHARACTER" usage which is there when malloc fails. Need to return early so that we don't override the state bits later.
Attachment #8901548 - Flags: review?(ehsan)
Blocks: 1394719
Comment on attachment 8901548 [details] [diff] [review]
nsStringBuffer_in_textfragment_2.diff

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

::: dom/base/nsTextFragment.cpp
@@ +120,5 @@
> +        if (!m2b) {
> +          MOZ_CRASH("OOM!");
> +        }
> +        static_cast<char16_t*>(m2b->Data())[0] = 0xFFFD; // REPLACEMENT CHARACTER
> +        static_cast<char16_t*>(m2b->Data())[1] = char16_t(0);

Nit: instead of casting like this repeatedly, please make a local char16_t* variable and assign it to the return value of Data() and then use that with the array index operator in both lines.

(It would have been nice if nsStringBuffer provided a helper to return the char16_t string so that we could avoid all of these casts...)

@@ +376,3 @@
>      mState.mLength += aLength;
>      m2b = buff;
> +    static_cast<char16_t*>(m2b->Data())[mState.mLength] = char16_t(0);

Same nit about repeated casts.

@@ +393,1 @@
>      size_t size = mState.mLength + aLength;

You need to move the |+ 1| from |size + sizeof(char16_t)| below to here to keep the overflow check in the next line correct.

@@ +417,3 @@
>      }
>      m2b = buff;
> +    static_cast<char16_t*>(m2b->Data())[mState.mLength] = char16_t(0);

Same nit about casts.
Attachment #8901548 - Flags: review?(ehsan) → review+
ah, oops, I moved some other  + sizeof(char16_t) also to be before the size check
Attachment #8901516 - Attachment is obsolete: true
Attachment #8901548 - Attachment is obsolete: true
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/243e4dae5e85
use nsStringBuffer for nsTextFragment::m2b r=ehsan
https://hg.mozilla.org/mozilla-central/rev/243e4dae5e85
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #8)
> ::: dom/base/nsTextFragment.cpp
> @@ +120,5 @@
> > +        if (!m2b) {
> > +          MOZ_CRASH("OOM!");
> > +        }
> > +        static_cast<char16_t*>(m2b->Data())[0] = 0xFFFD; // REPLACEMENT CHARACTER
> > +        static_cast<char16_t*>(m2b->Data())[1] = char16_t(0);
> 
> Nit: instead of casting like this repeatedly, please make a local char16_t*
> variable and assign it to the return value of Data() and then use that with
> the array index operator in both lines.
> 
> (It would have been nice if nsStringBuffer provided a helper to return the
> char16_t string so that we could avoid all of these casts...)

I used to have a patch in my local tree that made Data() a template method, so you had to indicate which character type was being accessed.  Honestly, it didn't seem like that big of a win at the time...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: