nsCString::ToNewCString() causes unnecessary strlen()

VERIFIED FIXED

Status

()

Core
XPCOM
P3
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: troy, Assigned: rickg)

Tracking

({perf})

Trunk
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+], URL)

(Reporter)

Description

18 years ago
The nsCString::ToNewCString() code doesn't pass in the length when it constructs 
the automatic nsCString object, and this results in an unnecessary strlen() call

This is showing up in Quantify as a problem when loading http://www.amazon.com/, 
and I think we should fix this for beta1

char* nsCString::ToNewCString() const {
  nsCString temp(mStr);
  temp.SetCapacity(8);
  char* result=temp.mStr;
  temp.mStr=0;
  return result;
}
(Reporter)

Updated

18 years ago
Keywords: beta1, perf

Comment 1

18 years ago
Please explain the user impact and if serious, remove PDT+ for reconsideration.
Whiteboard: [PDT-]
(Reporter)

Comment 2

18 years ago
Come on this is ridiculous. If we don't pass along the length then we're going 
to have to call strlen() to compute it. If you look at the Quantify data for 
loading http://www.amazon.com/ you'll see we do 55,000 calls to strlen(). This 
is accounting for like 15,000 of the calls...

The fix is a one-line fix and Rick knows exactly what change to make because he 
has already changed the ToNewString() functions and this is the same basic 
change
Whiteboard: [PDT-] → [PDT]
(Assignee)

Comment 3

18 years ago
Adding to what Troy said -- not only do I have a well-tested, one-line fix in 
hand, but I actually had the fix in my tree last week on a similar perf issue 
that PDT approved. I failed to land it because my home/office tree's got out of 
sync on this one-liner.

It's an important change to make for perf reasons, and troy's quantify data 
shows, and since perf is so poor now, this can only help. 
Status: NEW → ASSIGNED

Comment 4

18 years ago
Putting on PDT+ radar for beta1.
Whiteboard: [PDT] → [PDT+]
(Assignee)

Comment 5

18 years ago
Fixed by landing change I had sitting in my tree. Also removed warnings.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 6

18 years ago
amazon.com page load has become greatly improved over last few builds.  Marking 
Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.