Closed Bug 190020 Opened 22 years ago Closed 22 years ago

GetResource should take AUTF8String

Categories

(Core Graveyard :: RDF, defect, P2)

Product:
Core Graveyard
Component:
RDF
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.4alpha

People

(Reporter: bzbarsky, Assigned: Biesinger)

References

Details

Attachments

(3 files, 2 obsolete files)

patch v2
40.76 KB, application/x-gzip
timeless
: review+
alecf
: superreview+
Details
diff output
1.56 KB, text/plain
Details
this should do it
3.32 KB, patch
timeless
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
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.
will take a look at this
Assignee: rjc → cbiesinger
Priority: -- → P2
Target Milestone: --- → mozilla1.4alpha
Hm, actually, is GetUnicodeResource still needed after this change? GetResource
could be used instead of it...
It's not _needed_, but it could be left in as a convenience method....
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
("it" in my last comment being GetUnicodeResource)
yeah, making it AString sounds just fine.
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.
Depends on: 181136
Attached file gzipped patch (obsolete) — 
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
Attachment #113429 - Attachment is patch: false
Attachment #113429 - Attachment mime type: text/plain → application/x-gzip
Attachment #113429 - Flags: review?(alecf)
Status: NEW → ASSIGNED
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 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.
Attachment #113429 - Flags: review?(alecf)
Attached file 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.
Attachment #113429 - Attachment is obsolete: true
Attachment #113854 - Attachment is patch: false
Attachment #113854 - Attachment mime type: text/plain → application/x-gzip
Attachment #113854 - Flags: review?(alecf)
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... 
just perusing the patch, this really looks like great stuff!

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...

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.
Attached file diff output — 
I find it interesting that all size differences are a multiple of 4 KB...
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 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
Attachment #113854 - Flags: review?(alecf) → superreview+
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.
Attachment #113854 - Flags: review?(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).
Attachment #113854 - Flags: review?(bzbarsky) → review?(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)
Attachment #113854 - Flags: review?(timeless) → review+
Nice. I'm satisfied with that. (and great catch on the mem-alloc failure case!)
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!
alec, do you think we need to re-spin to test before opening the tree, or is
green tinderbox good enough for you?
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.
affirmative, i'll update tinderbox and let the world know.
OK. Patch checked in. I'll wait with marking this fixed until it cycles on the
tboxes.
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.
everything's green (after some bustage fixes), marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Hm, should iterators or nsCAutoString increase the code by as much as 2 KB?
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.
this patch should fix the charset menu, and all other places where rdf
vocabulary is used. reviews?
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.  ;)
Attachment #115700 - Flags: superreview+
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?
Attachment #115700 - Flags: review?
Comment on attachment 115700 [details] [diff] [review]
this should do it

oops, one more try.. but I'll take a review from anyone..
Attachment #115700 - Flags: review? → review?(jaggernaut)
Attachment #115700 - Flags: review?(jaggernaut) → review+
oops, sorry, I didn't realize that these were char* instead of char[]

thanks for fixing this!
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?


>Why was the null ptr check removed anyway?

Which null ptr check?
Attached patch fix mingw build, hopefully (obsolete) — — Splinter Review 
cls: This patch should fix it, and is the right thing, too, I think.
Comment on attachment 116032 [details] [diff] [review]
fix mingw build, hopefully

wfm. r=cls
Attachment #116032 - Flags: review+
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.
Attachment #116032 - Flags: superreview?(alecf)
QA Contact: tever → nobody
Comment on attachment 116032 [details] [diff] [review]
fix mingw build, hopefully

sr=alecf
Attachment #116032 - Flags: superreview?(alecf) → superreview+
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.
Attachment #116032 - Attachment is obsolete: true
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: