Four line patch to cut memcpy calls in half

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
17 years ago
5 years ago

People

(Reporter: jband_mozilla, Assigned: jag-mozilla)

Tracking

({perf})

Trunk
Future
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 57396 [details] [diff] [review]
CopyChars2To2 patch to avoid memcpy call most of the time
(Reporter)

Updated

17 years ago
Keywords: perf
(Assignee)

Comment 2

17 years ago
Hmmm. Who is doing all these 1 char calls?
(Reporter)

Comment 3

17 years ago
Looks like nsCSSScanner calling nsStr::StrAppend from three different methods.
(Assignee)

Comment 4

17 years ago
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.

(Reporter)

Comment 5

17 years ago
patch?

Comment 6

17 years ago
Any news on that one?! 
Keywords: mozilla0.9.9
(Reporter)

Comment 7

17 years ago
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?
(Assignee)

Comment 8

17 years ago
I'm all for checking your patch in. sr=jag on the patch. Wanna take the bug?
(Reporter)

Comment 9

17 years ago
taking. Anyone for an r= ?
Assignee: scc → jband
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
Attachment #57396 - Flags: superreview+
Attachment #57396 - Flags: review+
(Reporter)

Comment 11

17 years ago
I checked in my patch. Reassign to jag in case he wants to do a more general fix
as he suggested.
Assignee: jband → jaggernaut
(Assignee)

Comment 12

17 years ago
-> future
Target Milestone: --- → Future

Comment 13

13 years ago
Is there any further work on that bug still intended?

Comment 14

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

This code is obsolete now!
Attachment #57396 - Attachment is obsolete: true

Comment 15

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

Marking 'WONTFIX'
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.