311458 - Appending a single char to a string does a memcpy

Status: RESOLVED DUPLICATE of bug 312681

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It looks like we treat a single char as a length-one char buffer... would it
possibly be faster to just assign to the right location instead of doing a
memcpy (via char_traits::copy)?

Comment 1

[Darin Fisher]

Sure... do we have any data to prove that this may be worth the effort?

Comment 2

[Benjamin Smedberg]

Where is this code? It sounds like it might be better for the CSS code to
construct a local dependentconcatenation and then append it at once.  Of course
the dependentconcatenation code doesn't deal with single chars (perhaps a
special single-char class could be arranged, I'm not very familiar with the
internals there).

Comment 3

[Boris Zbarsky [:bzbarsky]]

I was looking at a profile that does a bunch of canvas operations... setting the
fill color looks like this (all numbers are hits under the method):

24020 nsCanvasRenderingContext2D::SetFillStyle(nsIVariant*)

of which

14334 CSSParserImpl::ParseColorString

of which

1365 nsSubstring::Replace

That's about 10% of the total CSS parse time here.  For general CSS it's a bit
better, but not much -- still about 5%, as I recall.

I'm not sure how much of this is bug 311456 though, so maybe we should fix that
first...

Comment 4

[Boris Zbarsky [:bzbarsky]]

The code in question is nsCSSScanner::GatherIdent.  Between pushback and
character escapes, this might be nontrivial to do in a way that doesn't involve
appending a char at a time.

Comment 5

[Darin Fisher]

So, you believe that a large portion of the cost is the memcpy call done by
Replace?  Enough to warrant an additional codepath in the string module?

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, I dug about a bit, and it looks like nsSubstring::Replace breaks down as
(1365 hits total):

529 hits in the function itself
612 hits under nsSubstring::ReplacePrep(unsigned, unsigned, unsigned)
190 hits under memcpy

So yeah, memcpy doesn't seem to be a big deal here.  I'll reprofile once we
decide what we're doing with bug 311456, if anything.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Probably not really an issue, based on more profile-reading.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Looks like this got fixed after all.

Comment 9

[Boris Zbarsky [:bzbarsky]]

*** This bug has been marked as a duplicate of 312681 ***