JM: store PIC code separately from normal code

RESOLVED INVALID

Status

()

Core
JavaScript Engine
RESOLVED INVALID
7 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Normal code and IC code is currently interleaved in the same executable chunks.  But ICs (PICs, at least) get purged on every GC.  This leaves behind holes in the chunks.

Now, the problem isn't too bad because ICs are very small, 10s of bytes, compared to normal code chunks which are 100s or 1000s of bytes.  Furthermore, normal code chunks are discarded if unused for a few minutes.  But it's still probably worth avoiding.

The fix should be straightforward:  store IC code (or maybe just PIC code?) in separate execPools from normal code.
(Assignee)

Comment 1

7 years ago
And I just realized that if PIC code is always stored in separate execPools, then purging PICs on GC is much easier -- we can iterate through the PIC execPools rather than iterating through each PICInfo.  Furthermore, each PIC won't need to track its own execPools!

So: simpler, faster and uses less memory :)
(Assignee)

Comment 2

7 years ago
Oh, we do still have to iterate through each PICInfo in order to repatch it.
Summary: JM: avoid fragmentation caused by interleaving normal code and IC code → JM: store PIC code separately from normal code
(Assignee)

Comment 3

7 years ago
Created attachment 509328 [details] [diff] [review]
patch (against TM 61559:a2afd41ae041)

This patch just does the "store PIC code separately from other code" change.  It doesn't remove BasePolyIC::execPools nor free the PIC pools separately.  That's a bigger and riskier change, and one that might benefit from bug 631106 happening first.

dvander, feel free to delegate the review to someone else if you like.
Attachment #509328 - Flags: review?(dvander)
(Assignee)

Updated

7 years ago
Blocks: 615199
I really like this idea! Can we have an enum, like { JITPool_Main, JITPool_IC } instead of the boolean parameter? I find boolean parameters to be kind of confusing.
(Assignee)

Comment 5

7 years ago
Created attachment 510902 [details] [diff] [review]
patch, v2 (against TM 61845:53763d7e9e54)

Same as the last patch, but with the boolean replaced by PoolKind.
Attachment #510902 - Flags: review?(dvander)
(Assignee)

Updated

7 years ago
Attachment #509328 - Attachment is obsolete: true
Attachment #509328 - Flags: review?(dvander)
Attachment #510902 - Flags: review?(dvander) → review+
(Assignee)

Updated

7 years ago
Blocks: 637216
(Assignee)

Updated

7 years ago
Blocks: 640457
(Assignee)

Comment 6

7 years ago
Note that this patch will decrease fragmentation, but may end up using more memory because we may end up using more ExecPools.  Eg. in a compartment with a small amount of code we might end up allocating two ExecPools (64KB each) instead of one.  This needs per-compartment measurements for better evaluation (bug 661474).
Depends on: 661474
(Assignee)

Updated

7 years ago
Whiteboard: [MemShrink:P3]
I don't know what we're doing in ion and baseline, but I hear we have 0 issues with PIC code storage in JM, nowadays.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.