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

RESOLVED FIXED in mozilla0.9.8

Status

()

defect
P3
normal
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: darin.moz, Assigned: darin.moz)

Tracking

({perf})

Trunk
mozilla0.9.8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

1.87 KB, patch
jag+mozilla
: superreview+
Details | Diff | Splinter Review
Assignee

Description

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

Updated

18 years ago
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P3
Target Milestone: --- → mozilla0.9.8
Assignee

Comment 1

18 years ago
Posted patch v1.0 patch (obsolete) — Splinter Review
Assignee

Comment 2

18 years ago
i haven't measured the performance impact of this patch yet.
Assignee

Comment 3

18 years ago
dp: can you r= this patch?
jag: can you sr= this patch?

thx!

Comment 4

18 years ago
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.
Attachment #61669 - Flags: superreview+
Assignee

Comment 5

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

Comment 6

18 years ago
Yes.
Assignee

Updated

18 years ago
Blocks: 7251
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
Attachment #61669 - Flags: superreview+ → review+
Assignee

Comment 8

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

i think i'll do that instead.
Assignee

Comment 9

18 years ago
Posted patch v1.1Splinter Review
this patch is better :)
Attachment #61669 - Attachment is obsolete: true
Assignee

Comment 10

18 years ago
with this patch i noticed a consistent 0.3% startup improvement.
Assignee

Comment 11

18 years ago
i should have mentioned that my perf measurement was done using an optimized
linux build.
Comment on attachment 61780 [details] [diff] [review]
v1.1

wow. We could really misuse strings...
Attachment #61780 - Flags: review+
I wish we can do away with the alloc/free for getspec() and have some
GetSharedSpec()... sigh!
Assignee

Comment 14

18 years ago
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 :(
Assignee

Comment 15

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

Comment 16

18 years ago
jag: can you sr= this latest patch?  thx!
Keywords: patch
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
Attachment #61780 - Flags: superreview+
Assignee

Comment 18

18 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.