Closed Bug 1451524 Opened 6 years ago Closed 6 years ago

DLL interceptors should share trampoline VM

Categories

(Core :: General, enhancement, P1)

Unspecified
Windows
enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: bugzilla, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Oh, and another bad thing about the current state: The trampoline space can't ever be freed; interceptors by design must leak their trampolines.
(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. :-)
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)
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+
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
Attachment #8971039 - Flags: review?(davidp99) → review+
Attachment #8971321 - Flags: review?(davidp99) → review+
I'm going to land the fixes to the shared policy but hold off on the default changeover until after the soft code freeze.
https://hg.mozilla.org/integration/mozilla-inbound/rev/501e46865f4327f5e104d4db22d644e54effc30f
Bug 1451524: Switch the default interceptor VM policy over from unique to shared; r=handyman
https://hg.mozilla.org/mozilla-central/rev/501e46865f43
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.