DLL interceptors should share trampoline VM

RESOLVED FIXED in Firefox 62

Status

()

P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla62
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Currently the Windows DLL interceptor's memory usage is very inefficient.

Each instance independently allocates its own VM for trampoline space. By default, each instance of the interceptor commits one page of VM, despite the fact that each instance sets no more than a couple of hooks.

Furthermore, since Windows allocates VM on a 64KiB granularity, we lose an additional 12KiB of address space per instance due to fragmentation, which is especially bad for 32-bit builds.

Once bug 1432653 lands, we can easily swap out policies for VM allocation such that most interceptor instances should be able to share a single trampoline space.

The code is already written for this, I'm just splitting out this bug from the existing refactoring work.
(Assignee)

Comment 1

a year ago
Oh, and another bad thing about the current state: The trampoline space can't ever be freed; interceptors by design must leak their trampolines.
(Assignee)

Comment 2

a year ago
(In reply to Aaron Klotz [:aklotz] from comment #0)
> Furthermore, since Windows allocates VM on a 64KiB granularity, we lose an
> additional 12KiB of address space per instance due to fragmentation, which
> is especially bad for 32-bit builds.

I think some of that beer I drank on my Mexico trip is still in my veins. Obviously we're losing 60KiB to fragmentation, not 12KiB. :-)
(Assignee)

Comment 3

a year ago
I don't trust MozReview to properly treat this patch independently from bug 1432653, so I'm using Splinter here...
Attachment #8967172 - Flags: review?(davidp99)
(Assignee)

Comment 4

11 months ago
Rebased atop latest revision of bug 1432653
Attachment #8967172 - Attachment is obsolete: true
Attachment #8967172 - Flags: review?(davidp99)
Attachment #8968633 - Flags: review?(davidp99)
Comment on attachment 8968633 [details] [diff] [review]
Add VM sharing policy for sharing trampoline space between interceptors (r2)

Review of attachment 8968633 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8968633 - Flags: review?(davidp99) → review+
(Assignee)

Comment 6

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/27d5c7a1d4c112687b78b4a9f86857378bfc67bb
Bug 1451524: Add a VM policy to the DLL interceptor that allows multiple instances to share a single trampoline space; r=handyman
(Assignee)

Updated

11 months ago
Keywords: leave-open
Attachment #8971039 - Flags: review?(davidp99) → review+
Attachment #8971321 - Flags: review?(davidp99) → review+
(Assignee)

Comment 10

11 months ago
I'm going to land the fixes to the shared policy but hold off on the default changeover until after the soft code freeze.
(Assignee)

Comment 11

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/83d635a4720194bec17d98035878d33cca1aa0e8
Bug 1451524: Make interceptor shared VM policy compatible with changes from bug 1456054; r=handyman
(Assignee)

Updated

11 months ago
Keywords: leave-open
(Assignee)

Comment 14

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/501e46865f4327f5e104d4db22d644e54effc30f
Bug 1451524: Switch the default interceptor VM policy over from unique to shared; r=handyman

Comment 15

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/501e46865f43
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
status-firefox61: affected → wontfix
You need to log in before you can comment on or make changes to this bug.