VMPI_getPrivateResidentPageCount is expensive - can we optimize?

NEW
Unassigned

Status

--
minor
9 years ago
7 years ago

People

(Reporter: stejohns, Unassigned)

Tracking

unspecified
Future
Bug Flags:
flashplayer-qrb +

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
Currently, we call VMPI_getPrivateResidentPageCount() from GCHeap::ExpandHeap(), to maintain the maxPrivateMemory field. Some implementations of this VMPI call are slow; on Windows, we attempt walk the entire address space (!) and call VirtualQuery on each section.

(1) Is it possible to have GCHeap / VMPI maintain the private resident page count itself, or must we query the OS every time?

(2) can we call this less often?
(Reporter)

Comment 1

9 years ago
Created attachment 400590 [details] [diff] [review]
Patch #1 -- conditionalize maxPrivateMemory

This is a bit of a hack, but the maxPrivateMemory field is only needed when MMGC_POLICY_PROFILING is defined (which it usually isn't). Commenting this out removes the need for the VMPI call in expandHeap -- in a testcase that is ExpandHeap-happy, it improves performance by ~5% (on my Vista64 machine).
Attachment #400590 - Flags: superreview?(lhansen)
Attachment #400590 - Flags: review?(treilly)

Updated

9 years ago
Attachment #400590 - Flags: review?(treilly) → review+

Comment 2

9 years ago
Comment on attachment 400590 [details] [diff] [review]
Patch #1 -- conditionalize maxPrivateMemory

I approve this for the sake of expediency, but the change is in some sense incorrect.  The API was added to GCHeap to allow the Flash Player to read consistent data for memory use, something previous APIs did not allow.  See eg comment 15 on bug #501318, although it's not very informative.  Anyhow the API landed without changes to the player to make use of it, IIRC, so this is probably OK for now.

(Be aware that MMGC_POLICY_PROFILING is enabled by default in release builds of the avm shell.  This is by design, but it's debatable whether it's smart.)
Attachment #400590 - Flags: superreview?(lhansen) → superreview+
(Reporter)

Comment 3

9 years ago
(In reply to comment #2)
> (From update of attachment 400590 [details] [diff] [review])
> I approve this for the sake of expediency, but the change is in some sense
> incorrect. 

Tommy suggested making this a runtime-configurable option (via a GCHeapConfig var)... would that change be preferable?

Also, is it possible to track this number directly, rather than having to ask the OS?
(Reporter)

Comment 4

9 years ago
Comment on attachment 400590 [details] [diff] [review]
Patch #1 -- conditionalize maxPrivateMemory

pushed as changeset:   2504:7419870a4407

Updated

9 years ago
Priority: -- → P5
Target Milestone: --- → flash10.1

Updated

9 years ago
Target Milestone: flash10.1 → flash10.2

Comment 5

9 years ago
Deferring, as the cost is not a problem if it's not called much.
Priority: P5 → --
Summary: VMPI_getPrivateResidentPageCount is expensive → VMPI_getPrivateResidentPageCount is expensive - can we optimize?
Target Milestone: flash10.2 → Future

Updated

7 years ago
Severity: normal → minor
Flags: flashplayer-qrb+
You need to log in before you can comment on or make changes to this bug.