115164 - nsJARURI::FormatSpec does more work than it needs to do

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Darin Fisher]

nsJARURI::FormatSpec, which is called by nsJARURI::GetSpec and nsJARURI::Resolve
does between 1-2 more strdup's than necessary.

Comment 1

[Darin Fisher]

Attachment: v1.0 patch

Comment 2

[Darin Fisher]

i haven't measured the performance impact of this patch yet.

Comment 3

[Darin Fisher]

dp: can you r= this patch?
jag: can you sr= this patch?

thx!

Comment 4

[jag (Peter Annema)]

Comment on attachment 61669 [details] [diff] [review]
v1.0 patch

>Index: nsJARURI.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/protocol/jar/src/nsJARURI.cpp,v
>retrieving revision 1.30
>diff -u -r1.30 nsJARURI.cpp
>--- nsJARURI.cpp	6 Dec 2001 01:20:33 -0000	1.30
>+++ nsJARURI.cpp	14 Dec 2001 01:39:50 -0000
>@@ -72,24 +72,23 @@
>     return NS_OK;
> }
> 
>-#define NS_JAR_SCHEME           "jar:"
>-#define NS_JAR_DELIMITER        "!/"
>+#define NS_JAR_SCHEME           NS_LITERAL_CSTRING("jar:")
>+#define NS_JAR_DELIMITER        NS_LITERAL_CSTRING("!/")
> 
> nsresult
> nsJARURI::FormatSpec(const char* entryPath, char* *result)
> {
>     nsresult rv;
>-    char* jarFileSpec;
>-    rv = mJARFile->GetSpec(&jarFileSpec);
>+
>+    nsXPIDLCString jarFileSpec;
>+    rv = mJARFile->GetSpec(getter_Copies(jarFileSpec));
>     if (NS_FAILED(rv)) return rv;
> 
>-    nsCAutoString spec(NS_JAR_SCHEME);
>-    spec += jarFileSpec;
>-    nsCRT::free(jarFileSpec);
>-    spec += NS_JAR_DELIMITER;
>-    spec += entryPath;
>-    
>-    *result = ToNewCString(spec);
>+    *result = ToNewCString(NS_JAR_SCHEME +
>+                           jarFileSpec +
>+                           NS_JAR_DELIMITER +
>+                           nsDependentCString(entryPath));
>+
>     return *result ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
> }
> 
>@@ -140,7 +139,7 @@
>     // contained within the bar.jar file.
> 
>     nsCAutoString jarPath(aSpec);
>-    PRInt32 pos = jarPath.RFind(NS_JAR_DELIMITER);
>+    PRInt32 pos = jarPath.RFind(NS_JAR_DELIMITER.get());

If you don't want to do this, you can use NS_LITERAL_CSTRING(NS_JAR_SCHEME)
above
instead of changing the #defines.

sr=jag either way.

Comment 5

[Darin Fisher]

jag: does the .get() on a literal string result in virtual function calls?

Comment 6

[jag (Peter Annema)]

Yes.

Comment 7

[Suresh Duddi (gone)]

Comment on attachment 61669 [details] [diff] [review]
v1.0 patch

XPIDLCString does a 4 byte alloc. Hence I dont like it.  :-(

I will leave it to you if you wanna convert back to non-XPIDLCString

r=dp

Comment 8

[Darin Fisher]

well... it'd be trivial to rewrite this to just use malloc and memcpy.

i think i'll do that instead.

Comment 9

[Darin Fisher]

Attachment: v1.1

this patch is better :)

Comment 10

[Darin Fisher]

with this patch i noticed a consistent 0.3% startup improvement.

Comment 11

[Darin Fisher]

i should have mentioned that my perf measurement was done using an optimized
linux build.

Comment 12

[Suresh Duddi (gone)]

Comment on attachment 61780 [details] [diff] [review]
v1.1

wow. We could really misuse strings...

Comment 13

[Suresh Duddi (gone)]

I wish we can do away with the alloc/free for getspec() and have some
GetSharedSpec()... sigh!

Comment 14

[Darin Fisher]

but doing so would require nsJARURI to also implement GetSharedSpec.  doing so
means significantly increasing the footprint of a nsJARURI.  not sure what's
better :(

Comment 15

[Darin Fisher]

a good compromise might be a GetSpec(nsAWritableCString &) method.  too bad this
wouldn't be scriptable :(

Comment 16

[Darin Fisher]

jag: can you sr= this latest patch?  thx!

Comment 17

[jag (Peter Annema)]

Comment on attachment 61780 [details] [diff] [review]
v1.1

That and you'd have to find a different name since you can't overload methods
in
idl.

Perhaps that const getter is still the best. Or a getter that generates the
spec into an existing buffer at the point you hand it.

Anyway, sr=jag

Comment 18

[Darin Fisher]

fixed-on-trunk