109562 - Four line patch to cut memcpy calls in half

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[John Bandhauer]

It turns out that literally 99% of our calls to CopyChars2To2 are to copy only 
one char. By avoiding a memcpy call for that simple one char case we can avoid 
roughly half the memcpy calls we make during startup and new window creation. 
I'm seing about 100,000 memcpy calls at startup skipped with this patch. They 
were cheap calls since only one char was being copied, but avoiding calls is 
good.

FWIW, The counterpart CopyChars1To1 call very rarely copies only one char. The 
places in string code using memmove don't seem to get called enough to matter.

It looks like this functionality is going to migrate to nsCharTraits::copy 
(right?). I think it will be worth adding this shortcut there when the code 
changes over.

Comment 1

[John Bandhauer]

Attachment: CopyChars2To2 patch to avoid memcpy call most of the time

Comment 2

[jag (Peter Annema)]

Hmmm. Who is doing all these 1 char calls?

Comment 3

[John Bandhauer]

Looks like nsCSSScanner calling nsStr::StrAppend from three different methods.

Comment 4

[jag (Peter Annema)]

Ah, looks like nsCSSToken::AppendToString is the main function doing this.

I wonder if it would be better to optimize
|nsAString::do_AppendFromElement( char_type aChar )| and its override,
|nsString::do_AppendFromElement( PRUnichar inChar )|, instead.

Better in the sense that we're not adding an |if| to CopyChars2to2, and better
in that we're addressing this issue for both old string code and new string
code.

Comment 5

[John Bandhauer]

patch?

Comment 6

[Markus Hübner]

Any news on that one?! 

Comment 7

[John Bandhauer]

The code in my patch has been running in my tree for a long time. Jag: unless
you are going to pursue a better patch soon, why not get mine into the tree in
the mean time rather than blowing off the whole thing for the promise of
something better?

Comment 8

[jag (Peter Annema)]

I'm all for checking your patch in. sr=jag on the patch. Wanna take the bug?

Comment 9

[John Bandhauer]

taking. Anyone for an r= ?

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 57396 [details] [diff] [review]
CopyChars2To2 patch to avoid memcpy call most of the time

Recording jag's sr= (or r= as module owner), and my r/sr=.

/be

Comment 11

[John Bandhauer]

I checked in my patch. Reassign to jag in case he wants to do a more general fix
as he suggested.

Comment 12

[jag (Peter Annema)]

-> future

Comment 13

[Markus Hübner]

Is there any further work on that bug still intended?

Comment 14

[Alfred Kayser]

Comment on attachment 57396 [details] [diff] [review]
CopyChars2To2 patch to avoid memcpy call most of the time

This code is obsolete now!

Comment 15

[Alfred Kayser]

nsCSSToken::AppendToString(nsString& aBuffer)  now uses the new string stuff, so this patch is obsolete.

Marking 'WONTFIX'