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

Does nsTextFragment::SetTo() now show up significantly in the profiles of bug 1346723?

If it's regressed, bug 1392181 may be the cause.

Oops, bug 1392181 must be safe. I meant bug 1391538.

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.

I'm at least investigating this.

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.

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.

ah, oops, I moved some other + sizeof(char16_t) also to be before the size check

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

(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...