92641 - nsIOService::NewURI strdup's the scheme when it doesn't need to

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

nsIOService::NewURI strdup's the scheme when it doesn't need to.  I have a patch
that avoid's strduping it for URI's with a scheme (I may extend to to some
relative URI's as well).  This is called circa 1100 times at startup according
to Suresh Duddi <[e-mail]>.

Comment 1

[Bradley Baetz (:bbaetz)]

->dougt, who was looking at string allocations (unless Randell wants to take this)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

I'll take it; I already have a patch coded up (will upload on monday).

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch to avoid extra allocation for most uses

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

patch posted; targeting 0.9.4.

Anyone who can, r/sr please.  According to the posts in .performance, there are
at least 1000 uses of this at startup, almost all of which are making an extra
allocation of the scheme, so this should save circa 1000 alloc/frees at startup.

Future work might be to make the scheme not be allocated in either absolute or
relative cases.

Comment 5

[Daniel Bratell]

+        if (end != 0 ? !nsCRT::strncasecmp(&scheme[start], gScheme[i],
+                                           (end-start)-1) :
+                       !nsCRT::strcasecmp(scheme, gScheme[i]))


That was a new construction to me. Took a couple of seconds to figure it out.
Why not:
        PRBool foundScheme;
        if (end == 0)
           foundScheme = !nsCRT::strcasecmp(scheme, gScheme[i])
        else
           foundScheme = !nsCRT::strncasecmp(&scheme[start], gScheme[i],
                                           (end-start)-1);

         if (foundScheme)


But the idea seems right.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Habit.  Totally readable to me to avoid an extra localvar.  Any r=/sr=, or if
reviewers object, I can recode as Daniel suggests.

Comment 7

[David Hyatt]

Seems like you could use an XPIDLCString for scheme and remove the free call.  
Other than that, sr=hyatt

Comment 8

[Doug Turner (:dougt)]

what hyatt said. r=dougt

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Updated patch using nsXPIDLCString

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

hyatt, dougt: Please either re-review or just tell me to check in.

The only change was to use an nsXPIDLCString as suggested.  I hadn't before
because the original code didn't use one (it did explicit frees).  Thanks.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Added waterson; he's my check-in buddy on this.  I think we're ready to check in.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Fix checked in