Reduce heap churn caused by calls to get_content_type_from_mime_type()

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Posted 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.
(Assignee)

Comment 2

5 years ago
Just some preliminary clean-up.
Attachment #8506636 - Flags: review?(karlt)
(Assignee)

Comment 3

5 years ago
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.

[1] https://developer.gnome.org/gio/2.42/gio-GContentType.html#g-content-type-from-mime-type
Attachment #8506637 - Flags: review?(karlt) → review-
Duplicate of this bug: 795169
(Assignee)

Comment 7

5 years ago
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)
(Assignee)

Updated

5 years ago
Attachment #8506637 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8506636 - Attachment is obsolete: true
Attachment #8507600 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/b08a0ab5efdf
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Updated

5 years ago
You need to log in before you can comment on or make changes to this bug.