28837 - nsCString::ToNewCString() causes unnecessary strlen()

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[troy]

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;
}

Comment 1

[daver]

Please explain the user impact and if serious, remove PDT+ for reconsideration.

Comment 2

[troy]

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

Comment 3

[rickg]

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. 

Comment 4

[leger]

Putting on PDT+ radar for beta1.

Comment 5

[rickg]

Fixed by landing change I had sitting in my tree. Also removed warnings.

Comment 6

[leger]

amazon.com page load has become greatly improved over last few builds.  Marking 
Verified.