1084193 - Reduce heap churn caused by calls to get_content_type_from_mime_type()

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Nicholas Nethercote [inactive]]

get_content_type_from_mime_type() is amazingly expensive. The calls to it just
at browser start-up account for over 45 MiB of cumulative allocations, which is
over 10% of all cumulative heap allocations at start-up.

We can easily halve this: nsGNOMERegistry::GetFromType() calls
GetAppForMimeType() and GetDescriptionForMimeType(), both of which call
get_content_type_from_mime_type() and then free the structure it returns. We
can add a new function GetAppAndDescriptionForMimeType() that gets both the
app and the description with a single get_content_type_from_mime_type() call.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: DMD output

I found this by doing some heap profiling with DMD configured to ignore all
calls to free() and |delete|. This means it profiles all heap allocations,
rather than just the live ones. Among other things, this identifies areas of
high heap churn. The relevant data is in the attachment.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Simplify some control flow in nsGIOService.cpp

Just some preliminary clean-up.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Reduce heap churn caused by calls to get_content_type_from_mime_type()

This fixes the problem. The code duplication isn't ideal.
nsIGIOService.getDescriptionForMimeType() is no longer used within the
codebase, so if we don't think there are any external consumers of note we
could remove that, which would help.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8506636 [details] [diff] [review]
(part 1) - Simplify some control flow in nsGIOService.cpp

Yes, handling errors where they happen is easier to follow thanks.

>-  if (!content_type)
>+  if (!content_type) {
>     return NS_ERROR_FAILURE;
>+  }

I don't mind unbraced jump statements in "if" statements, because it is hard to get any nested if statements wrong with jump statements, so usually I'd leave the code as is for the sake of history.

However, I'm happy for these changes to related code here, given a general desire amongst developers for consistent styles.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8506637 [details] [diff] [review]
(part 2) - Reduce heap churn caused by calls to get_content_type_from_mime_type()

Thanks for the analysis here, Nicholas.

I'm going to r- adding further code using get_content_type_from_mime_type because we can avoid this altogether.

That helper function was added to make this code work against pre-2.18 version of GIO.  However, now configure requires 2.20 and so we have g_content_type_from_mime_type() [1] available.

g_content_type_from_mime_type looks much more efficient.
IIUC the API changes in this patch are just to workaround the inefficiency of get_content_type_from_mime_type, and so I'd prefer to stick with the existing interface unless there is still a problem with g_content_type_from_mime_type.

[1] https://developer.gnome.org/gio/2.42/gio-GContentType.html#g-content-type-from-mime-type

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Remove get_content_type_from_mime_type(), and replace calls to it with calls to g_content_type_from_mime_type()

Much nicer! I confirmed with DMD that the allocations have gone away.

I also removed the style changes; they're not worth the effort/churn when they
don't make the substantive change easier.

Comment 8

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3d0c0dd96b4e

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b08a0ab5efdf

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b08a0ab5efdf