Closed Bug 1084193 Opened 6 years ago Closed 6 years ago

Reduce heap churn caused by calls to get_content_type_from_mime_type()


(Toolkit :: General, defect)

Not set





(Reporter: njn, Assigned: njn)




(2 files, 2 obsolete files)

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.
Attached file 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.
Just some preliminary clean-up.
Attachment #8506636 - Flags: review?(karlt)
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.
Attachment #8506637 - Flags: review?(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.
Attachment #8506636 - Flags: review?(karlt) → review+
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.

Attachment #8506637 - Flags: review?(karlt) → review-
Duplicate of this bug: 795169
Blocks: gio
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.
Attachment #8507600 - Flags: review?(karlt)
Attachment #8506637 - Attachment is obsolete: true
Attachment #8506636 - Attachment is obsolete: true
Attachment #8507600 - Flags: review?(karlt) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.