516526 - VMPI_getPrivateResidentPageCount is expensive - can we optimize?

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Steven Johnson]

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?

Comment 1

[Steven Johnson]

Attachment: 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).

Comment 2

[Lars T Hansen]

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.)

Comment 3

[Steven Johnson]

(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?

Comment 4

[Steven Johnson]

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

pushed as changeset:   2504:7419870a4407

Comment 5

[Lars T Hansen]

Deferring, as the cost is not a problem if it's not called much.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.