Closed Bug 981693 Opened 6 years ago Closed 6 years ago

Improve JIT code memory reporters

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

The Ion/Baseline/regexp/other counters we have on ExecutablePool are updated when we allocate memory, but not when we release it.

This is pretty misleading because even if we discard, say, all Baseline code, about:memory may show a non-zero value for runtime/code/baseline if something else is keeping the pool alive.
Whiteboard: [MemShrink]
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
Required a number of changes to get the exact size of the allocated buffer including padding bytes etc. Especially annoying for the JSC assembler (only used by Yarr these days).

But the result is that we can now assert in ~ExecutablePool that all counters are 0, this makes it really hard to leak or update the counters incorrectly.

Passes jit_test.py --ion on x86/x64/ARM.
Attachment #8389147 - Flags: review?(n.nethercote)
glob: Jan attached a patch, which I reviewed. I submitted my review and got a page saying something about a database error. And now the patch and the review are both gone :(

Jan: looks pretty good. The only comment of note was that the hard-coding of REGEXP_CODE in MacroAssemblerCodeRef::release() didn't look right. r=me conditional on either fixing that or explaining why it's valid.
Flags: needinfo?(glob)
(In reply to Nicholas Nethercote [:njn] from comment #3)
> glob: Jan attached a patch, which I reviewed. I submitted my review and got
> a page saying something about a database error. And now the patch and the
> review are both gone :(

we're experiencing some more database related issues.  the data isn't lost however, if you view this bug without being logged in (tip: private browsing) you'll see the patch and review.  needless to say the DBAs are busy trying to figure this out.
Flags: needinfo?(glob)
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Jan: looks pretty good. The only comment of note was that the hard-coding of
> REGEXP_CODE in MacroAssemblerCodeRef::release() didn't look right. r=me
> conditional on either fixing that or explaining why it's valid.

A lot of code in js/src/assembler/assembler (at least MacroAssembler*, LinkBuffer etc) is only used by Yarr these days. JM is gone and Baseline/Ion have their own macro assembler etc. I'll add a comment there and will assert in LinkBuffer::LinkBuffer that codeKind == REGEXP_CODE.
> I'll add a comment there and will assert
> in LinkBuffer::LinkBuffer that codeKind == REGEXP_CODE.

Lovely, thanks.
Comment on attachment 8389147 [details] [diff] [review]
Patch

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

Ah, the patch is back from DB limbo. Have an r+.
Attachment #8389147 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/2c7fac27ca58
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 990154
You need to log in before you can comment on or make changes to this bug.