Consider to use nsStringBuffer for nsTextFragment::m2b

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

If we used nsStringBuffer, getting the data should be a lot faster than the current memcpy. It would become basically just a simple AddRef.

Comment 1

2 years ago
Does nsTextFragment::SetTo() now show up significantly in the profiles of bug 1346723?
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)

Comment 8

2 years ago
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

Comment 11

2 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/243e4dae5e85
use nsStringBuffer for nsTextFragment::m2b r=ehsan

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/243e4dae5e85
Status: NEW → RESOLVED
Last Resolved: 2 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.