623281 - about:memory reporting for method JIT code space usage

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Shaver (:shaver emeritus)]

Attachment: memory reporter, only tested with default debug config

Seems to work, in shell and browser.  Shows us using ~17M on gmail, which mostly goes away after it closes, similarly on techcrunch.

It doesn't account for allocator fragmentation costs, though, so I'll do another round later.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 501378 [details] [diff] [review]
memory reporter, only tested with default debug config

ignoring dwrite/configure.in/etc. stuff, memory reporter wiring looks fine

Comment 2

[Mike Shaver (:shaver emeritus)]

Attachment: memory reporter, only tested with default debug config

Fixed the misdiff, still learning how to use bookmarks sorry.

Comment 3

[Nicholas Nethercote [inactive]]

finishThisUp() should use the new nonCodeSize() function!


> cx->runtime->mjitCodeSize -= jscr->nonCodeSize() + jscr->codeSize();

These names are confusing -- "codeSize" has two different meanings.  In jscr->codeSize() it's just the main code, excluding ICs and all that extra stuff.  In mjitCodeSize it includes the ICs.


This is a good feature, BTW, I think it should go into Fx 4.0.  I'm still worried about bug 598466 and bug 615199.

Comment 4

[Mike Shaver (:shaver emeritus)]

finishThisUp can't use the nonCodeSize function, though I thought that too: it needs to calculate the JITScript size in order to allocate it.  I thought about initializing the JITScript on the stack, using codeSize, and then copying it over, but I wasn't sure it was worth it.

Basically all the names in this patch are terrible, though; lemme ponder that a bit.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> finishThisUp can't use the nonCodeSize function, though I thought that too: it
> needs to calculate the JITScript size in order to allocate it.

Oh, I see;  it's because of e.g. mics.length() vs nMICs.  A comment in each place saying "keep this in sync with the other place" would be good, then.

BTW, is this patch against an oldish version?  nMapPairs doesn't exist any more;  that code has been changed a bit lately.

Comment 6

[Mike Shaver (:shaver emeritus)]

I have such a comment above the code in finishThisUp, but I'll add one on codeSize, too.

It's against yesterday's m-c, because I'm a chump.  I'll rebase to tm.

Comment 7

[Mike Shaver (:shaver emeritus)]

(In reply to comment #5)
> BTW, is this patch against an oldish version?  nMapPairs doesn't exist any
> more;  that code has been changed a bit lately.

I still see nNmapPairs in JITScript as of 6a5b1f1a2296 ?

Comment 8

[Mike Shaver (:shaver emeritus)]

Attachment: memory reporter, only tested with default debug config, updated to tm

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 501568 [details] [diff] [review]
memory reporter, only tested with default debug config, updated to tm

>+    /* Please keep in sync with JITScript::nonCodeBytes! */

It's not called nonCodeBytes any more!


>     size_t totalBytes = sizeof(JITScript) +
>                         sizeof(NativeMapEntry) * nNmapLive +

I know you are avoiding refactoring finishThisUp(), but maybe totalBytes and
totalSize could be renamed to use the main/aux naming convention?  That's
only a handful of lines to change.


>+/* Please keep in sync with Compiler::FinishThisUp. */

lower case 'f' in finishThisUp.

</nitpicking>

Comment 10

[Mike Shaver (:shaver emeritus)]

Comments fixed locally, thanks.  I'll let dvander weigh in on the totalSize/totalBytes renaming, if he likes this approach at all.

Comment 11

[Mike Shaver (:shaver emeritus)]

http://hg.mozilla.org/tracemonkey/rev/d6675a84589d

Comment 12

[Mike Shaver (:shaver emeritus)]

I recommend also taking the chaser http://hg.mozilla.org/tracemonkey/rev/8d7836a659e8 so that you can build.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/d6675a84589d