517083 - TM: introduce a temp allocator for allocations during recording and compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

This will reduce memory pressure on the long-lived code and data allocator.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

This patch introduces tempAllocs for regexp and the tracer, but doesn't actually use it yet. It had some memory corruption with the full patch, so I was trying to move over separate parts step by step from dataAlloc to tempAlloc.

Comment 3

[Graydon Hoare :graydon]

Attachment: update

This patch rebases, fixes the memory error (it was tiny) and commences using the tempAlloc for a few minor cases. Passes all tests.

I'd prefer to name these monAlloc and recAlloc, to more plainly identify the lifetimes they're associated with (monitor lifetime and recorder lifetime). If you concur, I'll change that before landing. Or suggest a similarly clear name. I don't think "dataAlloc" really expresses much more than "allocator".

Note that we can't *quite* move LIR into recAlloc just yet, but after a couple other bits of work it should be ready. I'll follow through that work on bug 497009 and bug 513276.

Comment 4

[Andreas Gal :gal]

Thanks for picking up my slack here graydon.

I like dataAlloc because it holds data associated with the code cache, which is codeAlloc. Both dataAlloc and codeAlloc have the same lifetime (long lived).

Separate of that I would propose lirAlloc, which then can be long lived or short lived, depending on whether we have recompilation or not (not yet, so it lives with the recorder soon).

Comment 5

[Graydon Hoare :graydon]

(In reply to comment #4)
> Thanks for picking up my slack here graydon.

eh? it's assigned to me now :)

> I like dataAlloc because it holds data associated with the code cache, which is
> codeAlloc. Both dataAlloc and codeAlloc have the same lifetime (long lived).

Ok. Is that an r+ then?

> Separate of that I would propose lirAlloc, which then can be long lived or
> short lived, depending on whether we have recompilation or not (not yet, so it
> lives with the recorder soon).

Already got the last of the skip()s moved out into allocators in wip patch, will be adding a liralloc shortly.

Comment 6

[Andreas Gal :gal]

Comment on attachment 402160 [details] [diff] [review]
update

Reviewing a bunch of changes I did myself. There should be a rule against this but there isn't =)

Comment 7

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/8be2e8fed090

Comment 8

[Robert Sayre]

This is a SunSpider regression on windows and linux.

Regression: Tp4 increase 0.97% on Linux TraceMonkey
Regression: SunSpider increase 1.17% on XP TraceMonkey

Comment 9

[Graydon Hoare :graydon]

(In reply to comment #8)
> This is a SunSpider regression on windows and linux.
> 
> Regression: Tp4 increase 0.97% on Linux TraceMonkey
> Regression: SunSpider increase 1.17% on XP TraceMonkey

Sigh. How talos manages to get a clear signal on linux, I will never know.

Thinking about this, I've a pretty good idea what's causing it (the mReserve part of the allocator; it'll cause TraceRecorder as a structure to grow by 64kb, until the day we have an actually-sane OOM-handler). 

Sorry. Will fix once I'm in the office (move the temp alloc to the monitor, pass in a reference, reset when deleting recorder).

Comment 10

[Graydon Hoare :graydon]

Attachment: fix the regresssion

This moves the temp allocators up to the monitor and just *resets* them when the recorders die. Avoids rapid reallocation of the 64kb temp space.

It's a bit of a kludge, but we can remove it if and when we determine a less ghastly OOM handler. Meanwhile it appears to undo the performance regression on the machines I can measure with here.

Comment 11

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/5f7a394ebb67

Comment 12

[Andreas Gal :gal]

Comment on attachment 402420 [details] [diff] [review]
fix the regresssion

> 
>-    CLS(VMAllocator)        dataAlloc;   // A chunk allocator for LIR.
>-    CLS(nanojit::CodeAlloc) codeAlloc;   // A general allocator for native code.
>+    CLS(VMAllocator)        dataAlloc;   /* A chunk allocator for LIR.    */
>+    CLS(VMAllocator)        tempAlloc;   /* A temporary chunk allocator.  */
>+    CLS(nanojit::CodeAlloc) codeAlloc;   /* An allocator for native code. */

Uber-nit: order should be code, data, temp imo.

>+
>+    ~RegExpNativeCompiler() {
>+        /* Purge the tempAlloc used during recording. */
>+        tempAlloc.reset();
>+    }

And now you know why I added reset() =)

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5f7a394ebb67

Comment 14

[Robert Sayre]

this could help fix some perf problems, and it's been stable, so taking on 192.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/62b03aeedfa9

Comment 15

[Robert Sayre]

perf regression fix:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/abf2f44642c1