190020 - GetResource should take AUTF8String

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

The impl of GetResource already assumes the string is UTF8 (see
GetUnicodeResource impl). The only thing gained by declaring it "string" in the
IDL is that calls from JS fail when there's no reason for them to fail.

The hard part here is, of course, changing all the C++ callers.  ;)  timeless? 
biesi?  Interested?  If not, let me know and I'll try to do it.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

will take a look at this

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Hm, actually, is GetUnicodeResource still needed after this change? GetResource
could be used instead of it...

Comment 3

[Boris Zbarsky [:bzbarsky]]

It's not _needed_, but it could be left in as a convenience method....

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

ok then.

If it is left in, is it ok with you to make it take a AString rather than a
wstring? Almost all callers do .get() currently:
http://lxr.mozilla.org/seamonkey/search?string=getunicoderes

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

("it" in my last comment being GetUnicodeResource)

Comment 6

[Boris Zbarsky [:bzbarsky]]

yeah, making it AString sounds just fine.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

bug 181136 should be fixed before this gets checked in, because regviewer uses
this function, and I'm not going to fix a file that will get removed soon anyway.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: gzipped patch

note - the regviewer patch that this bug depends upon needs to be fixed before
this gets checked in

also note that I gzipped the patch due to its size

Comment 9

[Alec Flett]

ouch. ok, I'll look into it. But we're getting very strict w.r.t. code footprint
bloat.

so before I review, I want to make sure that you're doing the right thing when
it comes to reducing code bloat..

i.e.
1) when an explicit conversion from UCS2 to UTF8 already existed, that we're now
calling GetUnicodeResource()
2) that we're not blindly converting strings that we're pretty sure are ASCII
3) that the surrounding code that might be dealing with raw strings, and could
be simplified now that we can use the string classes, is simplified.

Secondly, what is the change in binary size with this patch, on an optimized build?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 113429 [details]
gzipped patch

cancelling the review while I investigate the issues mentioned by alecf.

I did make sure that I followed 1), but I did not pay much attention to 2) and
3).

Also, I do not have a tree without the patch to compare the size to. I'll make
one.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

ok, follow alecf's requests

note that I did not want to change any .idl files here

as for the size:
chb@frodo:~/mozilla-rdf-getresource/mozilla$ du -scL dist/bin
31688	dist/bin
chb@frodo:~/mozilla-rdf-getresource/mozilla-without-patch$ du -scL dist/bin
31729	dist/bin

The second one is a "cp -a" of mozilla, which has the patch, followed by "patch
-p0 -R < patchname", followed by a "make".

It looks like the patch makes the code slightly smaller.

Comment 12

[Alec Flett]

Great, thanks for going back and looking over the patch.

it certainly does look smaller, but I'd like to see more data.

Try this:

with-patch> find dist/bin -name '*.so' | xargs du -s > with-patch.du
without-patch> find dist/bin -name '*.so' | xargs du -s > without-patch.du

diff -bw -u with-patch.du without-patch.du

that will tell us which DLLs grew, and which shrank...

Based on your initial data, I'll try to go review the patch... 

Comment 13

[Alec Flett]

just perusing the patch, this really looks like great stuff!

Comment 14

[Alec Flett]

in nsDownloadManager.cpp I see this:
   NS_ConvertUCS2toUTF8 utf8Path(path);
   
   nsCOMPtr<nsIRDFResource> downloadRes;
-  gRDFService->GetResource(utf8Path.get(), getter_AddRefs(downloadRes));
+  gRDFService->GetResource(utf8Path, getter_AddRefs(downloadRes));

Could you skip the UTF8 conversion here and put in the raw path?

I don't see any others like that... but I do see that GetLiteral() could use
some lovin' too (but in a different bug! this one is big enough)

..still reviewing...

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

ok. er. what's this now?

Firstly, I apologize for my misleading number. It included the removal of
regviewer, which is bug 181136 (it must be done for my patch to not break
compilation). That accounts for 24 KB of the difference.

Now, I see that NSS shrank by 4 KB with my patch. This is a surprise for me,
because I didn't touch it at all?

I also see that NSS is the only library that shrank in size. Which makes me
wonder where my original size win comes from.

....doing some detailed analysis...
ah:
4 more KB from regviewer.xpt
8 more KB from other regviewer stuff in chrome/
28 KB size difference in chromelist.txt!?

I therefore retract my earlier statement that my patch makes the build a bit
smaller.
It seems that it actually makes it 20 KB larger. (IMHO, we should still check
this in. The regviewer removal in the dependant bug offsets this footprint loss :) )
I'll attach the diff of with-patch.du and without-patch.du in a second.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: diff output

I find it interesting that all size differences are a multiple of 4 KB...

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, and for that DownloadManager question:
The UTF-8 Path is later used for accessing a hashtable:
  nsCStringKey key(utf8Path);
  if (mCurrDownloads.Exists(&key))
    mCurrDownloads.Remove(&key);

so, the UTF-8 path is needed anyway, so I wouldn't really gain anything by using
GetUnicodeResource here.

Comment 18

[Alec Flett]

Comment on attachment 113854 [details]
patch v2

the patch looks good. the one issue I'm wondering about is the whole thing with
mLastURIPrefix in nsRDFService - you're now keeping the whole nsCAutoString
around instead of just the last 16 bytes... I mean, 16 bytes was probably just
an arbitrary number, and you're probably improving the situation, but its
something ot watch for in case we have any performance hits from this.

other than that, great use of NS_LITERAL_CSTRING all over the place...

I'm fine with the growth you've listed. Not tremendously happy with it but I
think that this gives us a higher potential for performance improvement down
the line.
r/sr=alecf

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 113854 [details]
patch v2

>the patch looks good. the one issue I'm wondering about is the whole thing with
>mLastURIPrefix in nsRDFService - you're now keeping the whole nsCAutoString
>around instead of just the last 16 bytes

Well, yeah. I mean, it's a service, 48 bytes more won't matter much, I figured,
and I didn't want to change to a nsString with the heap allocation.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, man.  I won't have the cycles to review anything this size for at least 2-3
weeks, and that's if a miracle happens (see my review queue and consider the
fact that I am limiting myself to 6 hours of Mozilla work a week).

Comment 21

[timeless]

Comment on attachment 113854 [details]
patch v2

(From update of attachment 113854 [details])
for XUL_RESOURCE, please align the \'s

>+    *aMailListURI = ToNewCString(m_MailListURI);
check for alloc failure (the old code was broken)
>                return NS_OK;


>+  if (isMoveFolder)
>+    return DoCommand(database, NS_LITERAL_CSTRING(NC_RDF_MOVEFOLDER), folderArray, argumentArray);
>+  else
omit the else
>+    return DoCommand(database, NS_LITERAL_CSTRING(NC_RDF_COPYFOLDER), folderArray, argumentArray);


>+    PR_LOG(gLog, PR_LOG_DEBUG, ("rdfserv get-resource %s", PromiseFlatCString(aURI).get()));
>     // First, check the cache to see if we've already created and
>     // registered this thing.
>+    const nsAFlatCString& flatURI = PromiseFlatCString(aURI);
not that it matters, but you could move the PR_LOG line <here> and use flatURI
:)


>+            nsCAutoString contractID(NS_RDF_RESOURCE_FACTORY_CONTRACTID_PREFIX);
>+            contractID += Substring(begin, p);
I'm not certain, but i think that:

+	     nsCAutoString contractID;
+	     contractID =
NS_LITERAL_CSTRING(NS_RDF_RESOURCE_FACTORY_CONTRACTID_PREFIX) +
Substring(begin, p);
might be better (save a realloc/copy)

Comment 22

[Alec Flett]

Nice. I'm satisfied with that. (and great catch on the mem-alloc failure case!)

Comment 23

[Alec Flett]

Thinking about this a bit more, my sr= is contingent on you getting a carpool
for this. Not only do I want to avoid this overlapping with other checkins, but
this is going to take a while to rebuild and I'd like to avoid doing that while
the tree is open ...Thanks!

Comment 24

[Daniel (Leaf) Nunes]

alec, do you think we need to re-spin to test before opening the tree, or is
green tinderbox good enough for you?

Comment 25

[Alec Flett]

to be honest, I'd like to see the smoketests run.. this is a pretty pervasive
change and I'm mostly worried that this will expose some other bug that was
relying on a broken conversion (such as from JS) or something.

Comment 26

[Daniel (Leaf) Nunes]

affirmative, i'll update tinderbox and let the world know.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

OK. Patch checked in. I'll wait with marking this fixed until it cycles on the
tboxes.

Comment 28

[Alec Flett]

interesting that nsRDFService::GetResource grew by some 2k on linux.. thats the
bulk of the growth on embedded builds. I wonder if it has something to do with
the use of iterators, or the use of nsCAutoString as a prefix holder? it sure
would be nice to trim some of that back down though.

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

everything's green (after some bustage fixes), marking FIXED.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

Hm, should iterators or nsCAutoString increase the code by as much as 2 KB?

Comment 31

[Alec Flett]

ok, so we missed a whole bunch of ugliness here
you can't say

const char* kFoo = "abc";
NS_LITERAL_CSTRING(kFoo)

I've gotten assertions in nsCharsetMenu and nsWindowDataSource so far.

Comment 32

[Alec Flett]

Attachment: this should do it

this patch should fix the charset menu, and all other places where rdf
vocabulary is used. reviews?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 115700 [details] [diff] [review]
this should do it

I go to the bug to comment about assertions, and I find a patch.  ;)

Comment 34

[Alec Flett]

Comment on attachment 115700 [details] [diff] [review]
this should do it

cool.. if I can just get one more reviewer, we'll be set. this is easy stuff..
jag? string stuff?

Comment 35

[Alec Flett]

Comment on attachment 115700 [details] [diff] [review]
this should do it

oops, one more try.. but I'll take a review from anyone..

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

oops, sorry, I didn't realize that these were char* instead of char[]

thanks for fixing this!

Comment 37

[hacker formerly known as [e-mail]]

This checkin seems to have caused the mingw port to die when creating the first
profile (including the Application Data/Mozilla dir).  The problem seems to stem
from someone calling nsRDFService::GetResource() with an empty aURI string. 
That also causes nsDependentCString to throw the assertion at
http://lxr.mozilla.org/seamonkey/source/string/public/nsDependentString.h#65 and
we eventually die at
http://lxr.mozilla.org/seamonkey/source/rdf/base/src/nsRDFService.cpp#1021 .

The odd thing is that I haven't been able to reproduce this with a MSVC build. 
Why was the null ptr check removed anyway?

Comment 38

[hacker formerly known as [e-mail]]

Err, that nsDependentCString assertion was at
http://lxr.mozilla.org/seamonkey/source/string/public/nsDependentString.h#128 .

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

>Why was the null ptr check removed anyway?

Which null ptr check?

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: fix mingw build, hopefully

cls: This patch should fix it, and is the right thing, too, I think.

Comment 41

[hacker formerly known as [e-mail]]

Comment on attachment 116032 [details] [diff] [review]
fix mingw build, hopefully

wfm. r=cls

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 116032 [details] [diff] [review]
fix mingw build, hopefully

alecf, could you sr this additional patch? It checks if the URI of the resource
is empty at the top of the function, which seems like the "right thing" to me
and fixes a problem with mingw.

Comment 43

[Alec Flett]

Comment on attachment 116032 [details] [diff] [review]
fix mingw build, hopefully

sr=alecf

Comment 44

[Alec Flett]

also, seawood I'd be curious to see the stack that lead up to the assertion in
nsDependentString.h - its usually the caller of that line that is bogus.. I
fixed a bunch of nsDependentString assertions recently, but I'd like to know if
there are more.