212468 - rethink the hashtable usage in helperappservice

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

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

Currently, the only entries that ever end up in the hashtable are those from
defaultMimeEntries. which means that the table is rather useless, except to
speed up mime type lookups by extension.

so I would suggest removing that hashtable, removing rarely used entries from
defaultMimeEntries, and in GetMimeTypeFromExtension just doing a linear search
in defaultMimeEntries.

The entries in there could be in the order xul,xml,rdf,gif,jpg,css,htm,html

Comment 1

[Nicholas Allen]

If you're going to remove the ability to add new entries, why not just make a
gperf table out of the existing defaultMimeEntries?

Comment 2

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

well... even today, that hashtable is not being added to...

I'll consider that, but wouldn't that add a compile-time dependency on gperf?

Comment 3

[Nicholas Allen]

I think alecf or shaver might know how to insert gperfed code into the build so
that it can be built on machines without gperf.  Using gperf at all here is just
a suggestion.  Although I think lookup will beat a linear search here, it may be
over-optimization depending on how frequently this is called.

Comment 4

[Boris Zbarsky [:bzbarsky]]

It's called somewhat frequently at startup (like for every .xul, .css, .gif,
.rdf, .xml, etc in the chrome).  But would a lookup really beat a linear search
over 5-10 entries that we can order in order of frequency?

Comment 5

[Nicholas Allen]

Depends on how biased the frequencies are.  The tradeoff is around when the
expected number of lookups for linear search crosses two (assuming the perfect
hash code generator is good).

If the number of types really can be cut to five, then simplicity wins.  The
difference between the two in that case should be microseconds.

Comment 6

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

Attachment: patch

Comment 7

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

Comment on attachment 127752 [details] [diff] [review]
patch

Here's the count of the extensions used at startup on my system (Win32,
Classic)
GIF: 29
XML: 24
RDF: 10
XUL: 3

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127752 [details] [diff] [review]
patch

>+  { TEXT_PLAIN, "txt,text", "Text File", 'TEXT', 'ttxt' },
>+#ifdef MOZ_SVG
>+  { "image/svg+xml", "svg", "Scalable Vector Graphics", 'svg ', 'ttxt' },
>+#endif
.....

Hey, as long as you're changing the order of these, please put them in a
logical order?	Eg, JPEG and GIF should be somewhere near PNG....

Also, I see no reason to have that #ifdef there now that this is not in the "no
override possible" list.

Finally, please put 

>+  { APPLICATION_XPINSTALL, "xpi", "XPInstall Install", 'xpi*','MOSS' },

in the "no override possible" list too, just in case.

r=me with these changes.

Comment 9

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

Comment on attachment 127752 [details] [diff] [review]
patch

bz: ok, I'll make those changes

Comment 10

[Darin Fisher]

Comment on attachment 127752 [details] [diff] [review]
patch

lot's of code removal... very nice.  sr=darin

Comment 11

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

requested changes made. I made the order mostly alphabetical.

Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsE
xternalHelperAppService.cpp
new revision: 1.201; previous revision: 1.200
done
Checking in nsExternalHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.h,v  <--  nsExt
ernalHelperAppService.h
new revision: 1.46; previous revision: 1.45
done

Comment 12

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

oh right, to address the gperf question: I ended up getting 4 types that had to
be fast, so I used a normal array. (there are ~8 entries in that array, but for
the last 4, speed doesn't matter so much)