673158 - Separate regexp JIT code and normal mjit code in about:memory

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

In Bug 670689, bz found that most of the code listed under mjit-code actually comes from the regexp JIT.  If we can separate them in about:memory, it will be easier to find large uses of memory that are really from the regexp JIT.

Comment 1

[Andrew McCreight [:mccr8]]

This isn't strictly speaking DarkMatter, as the regexp code is already accounted for somewhat.

Comment 2

[Nicholas Nethercote [inactive]]

AFAICT regexp code chunks and normal mjit code chunks are both allocated out of the same ExecutableAllocator, and can be interleaved arbitrarily.  That makes separate accounting difficult :/

Comment 3

[Justin Lebar (not reading bugmail)]

Obligatory comment from someone who knows nothing about JS: You know when you allocate or free the chunk whether it's regexp or mjit?  So why can't you just keep a counter of how much is currently allocated?

Comment 4

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

I think we can keep accounting outside the ExecutableAllocator (on the regexp paths) that at least determines the contribution made by RegExps and their corresponding code chunks.

Comment 5

[Boris Zbarsky [:bzbarsky]]

One question that I have is whether it makes sense to allocate them out of the same allocator... Or is that just a consequence of the JSC bits we use?

Comment 6

[Nicholas Nethercote [inactive]]

Not sure what you mean by "makes sense".  If we had separate allocators that could increase peak memory usage, because the minimum code chunk that can be allocated is 64KB.  It might reduce fragmentation... but that assumes that regexps and normal code have significantly different lifetimes, and I'm not sure that's true.

Comment 7

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

(In reply to comment #5)

No good reason to use different allocators, IMO -- small executable allocations get allocated out of shared space to avoid excessive fragmentation, and consolidating to a single RWX page allocation path is a good thing. Do you have a particular benefit in mind?

Comment 8

[Boris Zbarsky [:bzbarsky]]

The "make sense" bit is precisely a question about whether lifetimes are sufficiently different that fragmentation would be reduced with separate allocation pools.

If we just start to expire regexp jitcode using the same heuristics as mjit code, there's probably no benefit to splitting them up.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #8)
> 
> If we just start to expire regexp jitcode using the same heuristics as mjit
> code, there's probably no benefit to splitting them up.

I agree.

Comment 10

[Nicholas Nethercote [inactive]]

Probably the best way to do this would be to add new fields mNormalSize and mRegexpSize to ExecutablePool to track the number of bytes of each kind allocated.  And you'd need to pass an extra argument in various places (e.g. ExecutablePool::alloc()) to indicate if an allocation is normal or regexp code.

Comment 11

[Sander Mathijs van Veen]

Attachment: Patch for separate rjit- and mjit-code counters

This patch is also the result of co-author Bas Weelinck (Dutch CS student).

Comment 12

[Sander Mathijs van Veen]

Attachment: Patch for separate rjit- and mjit-code counters v2

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 555925 [details] [diff] [review]
Patch for separate rjit- and mjit-code counters v2

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

I'm giving r-, but this is definitely on the right track :)  There are enough little fixes below that I'd like to see fixed.

Also, you've used default arguments throughout for the JITCodeType arguments.  I understand why you did this, and it does minimize the number of lines touched, but in this case (as in many cases) I think explicit is better than implicit -- it's hard for me to tell that you haven't missed any of the places where a RJIT_CODE value should be fed in.  If all the default arguments were made explicit every place where a JITCodeType is injected is explicit and so clearer.  So I'd like you to make that change.  Thanks!

::: js/src/assembler/assembler/LinkBuffer.h
@@ +68,2 @@
>      {
> +        m_type = code_type;

Nit: m_codeType is more descriptive than m_type, which could mean anything.

Nit: Use "codeType" instead of "code_type", camelCaps is generally preferred, even though there are plenty of exceptions.

And I usually use "kind" instead of "type" for this sort of thing, because "type" has such a strong other meaning in programming languages.  Might be worth doing here too.

::: js/src/assembler/jit/ExecutableAllocator.cpp
@@ +37,5 @@
>      m_allocator->releasePoolPages(this);
>  }
>  
>  size_t
> +ExecutableAllocator::getMjitCodeSize() const

You shouldn't need this function any more;  total mjit code size will just be the sum of the method, regexp, and unused sizes.

@@ +48,5 @@
>      return n;
>  }
>  
> +size_t
> +ExecutableAllocator::getMjitCodeUnused() const

You could merge getMjitCode{Unused,Method,Regexp} into a single function that returns three values by reference, to avoid repeated iterations over the exec pools.  (A similar change would be needed for the layers of callers above this function.)  Not crucial, though, I'll let you decide whether to adjust.

::: js/src/assembler/jit/ExecutableAllocator.h
@@ +80,5 @@
>  namespace JSC {
>  
>    class ExecutableAllocator;
>  
> +  enum JITCodeType {MJIT_CODE, RJIT_CODE};

How about:

  enum MjitCodeKind {METHOD_CODE, REGEX_CODE};

to match the names in about:memory?  (And "Mjit" is used elsewhere, and I said above I preferred "kind" to "type" :)

@@ +168,5 @@
>      enum ProtectionSetting { Writable, Executable };
>  
>  public:
> +    ExecutableAllocator(JITCodeType type = MJIT_CODE)
> +        : code_type(code_type)

Nit: "codeKind", again.

@@ +236,5 @@
>      static size_t pageSize;
>      static size_t largeAllocSize;
>  
> +    // True, if this allocator is used for Regexp JIT code.
> +    bool code_type;

This should be a MjitCodeKind enum, not a bool.

But, I don't think it's ever used?  If not, it can be removed.

::: js/src/assembler/jit/ExecutableAllocatorPosix.cpp
@@ +42,5 @@
>  {
>      void* allocation = mmap(NULL, n, INITIAL_PROTECTION_FLAGS, MAP_PRIVATE | MAP_ANON, VM_TAG_FOR_EXECUTABLEALLOCATOR_MEMORY, 0);
>      if (allocation == MAP_FAILED)
>          allocation = NULL;
> +    ExecutablePool::Allocation alloc = {reinterpret_cast<char*>(allocation), n};

Needless whitespace change, please revert.

::: js/src/assembler/jit/ExecutableAllocatorSymbian.cpp
@@ +53,5 @@
>  
>      TInt errorCode = codeChunk->CreateLocalCode(n, n);
>  
>      char* allocation = reinterpret_cast<char*>(codeChunk->Base());
> +    ExecutablePool::Allocation alloc = {allocation, n, codeChunk};

Ditto.

::: js/src/jscompartment.cpp
@@ +167,5 @@
>      return true;
>  }
>  
>  size_t
>  JSCompartment::getMjitCodeSize() const

This also shouldn't be necessary.

::: js/src/jscompartment.h
@@ +442,5 @@
>      }
>  
>      bool ensureJaegerCompartmentExists(JSContext *cx);
>  
>      size_t getMjitCodeSize() const;

This also shouldn't be necessary.

::: js/src/xpconnect/src/xpcjsruntime.cpp
@@ +1267,5 @@
>  PRInt64
>  GetCompartmentMjitCodeSize(JSCompartment *c)
>  {
>      return c->getMjitCodeSize();
>  }

This also shouldn't be necessary.

@@ +1354,5 @@
>  
>      // Get the compartment-level numbers.
>      curr->scripts = GetCompartmentScriptsSize(compartment);
>  #ifdef JS_METHODJIT
> +    curr->mjitCodeSize = GetCompartmentMjitCodeSize(compartment);

And this isn't necessary.

@@ +1805,5 @@
>  
>      ReportMemoryBytes0(MakeMemoryReporterPath(pathPrefix, stats.name,
> +                                              "mjit-code/regex"),
> +                       nsIMemoryReporter::KIND_NONHEAP, stats.mjitCodeRegexp,
> +    "Memory used by the regexp JIT to hold the compartment's generated code.",

You should be consistent (in the code and the strings) whether you use "regex" or "regexp".  I slightly prefer the latter, but I'll let you choose.

::: js/src/xpconnect/src/xpcpublic.h
@@ +213,5 @@
>      PRInt64 scripts;
>  #ifdef JS_METHODJIT
> +    PRInt64 mjitCodeMethod;
> +    PRInt64 mjitCodeRegexp;
> +    PRInt64 mjitCodeSize;

mjitCodeSize (again) isn't necessary.

Comment 14

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

(In reply to Nicholas Nethercote [:njn] from comment #13)
> You should be consistent (in the code and the strings) whether you use
> "regex" or "regexp".  I slightly prefer the latter, but I'll let you choose.

Drive by nit: we use regexp everywhere else in the engine and in the JS language. :-)

Comment 15

[Sander Mathijs van Veen]

Attachment: Patch for separate rjit- and mjit-code counters v3

Compared to the previous patch:
 - Merged getMjitCode* -> getMjitCodeStats.
 - Changed codeType -> codeKind.
 - Removed all default codeKind initializers.
 - Modified the test cases in assembler/TestMain.cpp.

Unfortunately, my request for access to the try servers is still pending. Therefore, I'm unable to test it on all platforms. However, I did try it on linux 32 and 64 bit.

Example snippet of about:memory after the patch:

│   ├──22,431,452 B (18.27%) -- compartment(https://mail.google.com/mail/?shva=1)
│   │  ├───8,970,240 B (07.30%) -- gc-heap
│   │  │   ├──4,499,440 B (03.66%) -- objects
│   │  │   ├──2,177,944 B (01.77%) -- arena-unused
│   │  │   ├──2,079,800 B (01.69%) -- shapes
│   │  │   ├────136,512 B (00.11%) -- strings
│   │  │   ├─────41,504 B (00.03%) -- arena-padding
│   │  │   └─────35,040 B (00.03%) -- arena-headers
│   │  ├───4,980,736 B (04.06%) -- mjit-code
│   │  │   ├──4,775,104 B (03.89%) -- method
│   │  │   ├────168,444 B (00.14%) -- regexp
│   │  │   └─────37,188 B (00.03%) -- unused
│   │  ├───4,067,248 B (03.31%) -- scripts
│   │  ├───1,471,024 B (01.20%) -- object-slots
│   │  ├───1,226,876 B (01.00%) -- mjit-data
│   │  ├─────972,832 B (00.79%) -- property-tables
│   │  ├─────327,128 B (00.27%) -- tjit-data
│   │  │     ├──155,736 B (00.13%) -- allocators-main
│   │  │     ├───97,392 B (00.08%) -- trace-monitor
│   │  │     └───74,000 B (00.06%) -- allocators-reserve
│   │  ├─────284,296 B (00.23%) -- string-chars
│   │  └─────131,072 B (00.11%) -- tjit-code

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 556461 [details] [diff] [review]
Patch for separate rjit- and mjit-code counters v3

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

This looks great, thanks!  Just a few nits.  I'll send it to the try server.

::: js/src/assembler/jit/ExecutableAllocator.h
@@ +80,5 @@
>  namespace JSC {
>  
>    class ExecutableAllocator;
>  
> +  enum JITCodeKind {METHOD_CODE, REGEXP_CODE};

Nit: add a space after '{' and before '}'.

"JITCodeKind" could become "CodeKind" without any loss of clarity, if you liked.

@@ +196,5 @@
>  
>      // alloc() returns a pointer to some memory, and also (by reference) a
>      // pointer to reference-counted pool. The caller owns a reference to the
>      // pool; i.e. alloc() increments the count before returning the object.
> +    void* alloc(size_t n, ExecutablePool** poolp, JITCodeKind type)

Nit: s/type/kind/

::: js/src/jscompartment.cpp
@@ +177,5 @@
> +    }
> +
> +    method = 0;
> +    regexp = 0;
> +    unused = 0;

Nit: I'd avoid the early return and put the zero assignments in an else-block.  That way you don't need a return statement at all.

::: js/src/xpconnect/src/xpcjsruntime.cpp
@@ +1286,1 @@
>  #endif  // JS_METHODJIT

Nit: you removed the blank line before this #endif, but there's a blank line after the corresponding #ifdef, so I'd put the blank line back in.

@@ +1795,5 @@
> +
> +    ReportMemoryBytes0(MakeMemoryReporterPath(pathPrefix, stats.name,
> +                                              "mjit-code/unused"),
> +                       nsIMemoryReporter::KIND_NONHEAP, stats.mjitCodeUnused,
> +    "Memory allocated by the method JIT but currently unused.",

Nit: "Memory allocated by the method and/or regexp JIT to hold the compartment's code, but which is currently unused."

Comment 17

[Mozilla RelEng Bot]

Try run for 3c4fbe32ab62 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=3c4fbe32ab62
Results (out of 153 total builds):
    success: 143
    warnings: 6
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3c4fbe32ab62

Comment 18

[Nicholas Nethercote [inactive]]

There were Android build failures:

/builds/slave/try-lnx-andrd/build/js/src/assembler/assembler/ARMAssembler.cpp: In member function 'void* JSC::ARMAssembler::executableAllocAndCopy(JSC::ExecutableAllocator*, JSC::ExecutablePool**, JSC::JITCodeKind)':
/builds/slave/try-lnx-andrd/build/js/src/assembler/assembler/ARMAssembler.cpp:575: error: no matching function for call to 'JSC::AssemblerBufferWithConstantPool<2048, 4, 4, JSC::ARMAssembler>::executableAllocAndCopy(JSC::ExecutableAllocator*&, JSC::ExecutablePool**&, JSC::JITCodeKind&)'
/builds/slave/try-lnx-andrd/build/js/src/assembler/assembler/AssemblerBufferWithConstantPool.h:197: note: candidates are: void* JSC::AssemblerBufferWithConstantPool<maxPoolSize, barrierSize, maxInstructionSize, AssemblerType>::executableAllocAndCopy(JSC::ExecutableAllocator*, JSC::ExecutablePool**) [with int maxPoolSize = 2048, int barrierSize = 4, int maxInstructionSize = 4, AssemblerType = JSC::ARMAssembler]

Looks like you just missed one of the executableAllocAndCopy() cases, should be an easy fix.

Comment 19

[Mozilla RelEng Bot]

Try run for d4f86e0072d2 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=d4f86e0072d2
Results (out of 21 total builds):
    exception: 1
    success: 15
    warnings: 4
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d4f86e0072d2

Comment 20

[Nicholas Nethercote [inactive]]

Android looks good (apart from the usual, expected test failures).  Sander, if you can attach your final patch I'll land it tomorrow.  Thanks.

Comment 21

[Sander Mathijs van Veen]

Attachment: Patch for separate rjit- and mjit-code counters

This is the final patch, which passes the Android tests (see tbpl results above).

Comment 22

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d9bbe2d0b569

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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