Closed Bug 1643171 Opened 1 month ago Closed 1 month ago

Add memory reporter for regexp Isolate


(Core :: JavaScript Engine, task, P3)




Tracking Status
firefox79 --- fixed


(Reporter: mccr8, Assigned: iain)



(2 files)

There's around 10,000 bytes of unreported memory hanging off of JSContext from v8::internal::Isolate, but I didn't really want to dig into it. That's not much memory, but it looks like RegExpStack, which hangs off of it, can get larger, so it might be good to add a reporter in case we have instances where the stack gets large.

v8::internal::Isolate is a shim layer between irregexp (our RegExp engine, which we share with V8) and the rest of SpiderMonkey. Taking a quick look at it, I think there are three parts of the Isolate that own memory: two segmented vectors that we use to store roots for V8's GC things, and RegExpStack.

In theory the segmented vectors can grow; in practice they're already probably too big, and I could shrink them down to like 1/8 of the current size and still almost never allocate a second segment. The regexp engine doesn't root that many things; the only thing I can think of where it's not a small constant number of roots is named captures, where we have one string per name.

What is involved in adding a memory reporter? It might be a bit fiddly, because RegExpStack is defined upstream.

Severity: -- → N/A
Priority: -- → P3

(In reply to Iain Ireland [:iain] from comment #1)

What is involved in adding a memory reporter? It might be a bit fiddly, because RegExpStack is defined upstream.

Thanks for the comments. Writing a memory reporter is not difficult. You can look at the SizeOfExcludingThis methods in the tree. When I started looking at this, I added a size_t IsolateSizeOfIncludingThis(Isolate* isolate, MallocSizeOf mallocSizeOf) method to RegExpAPI.h. That basically calls mallocSizeOf on all of the blocks that the isolate points to and owns, which asks the allocator how big the blocks are. Then it sums those up, allong with mallocSizeOf(isolate), and returns them.

Then, you need to make JSContext::sizeOfExcludingThis() call IsolateSizeOfIncludingThis and add it into the total.

It does sound like the memory could be improved a bit, although jandem's suggestion of lazily initializing JSContexts for helper threads would probably do much of the same thing here.

By default, SegmentedVector allocated in segments of 4096 bytes. That's incredible overkill for the arenas in Isolate that store roots and pseudo-roots, because the only place in irregexp where we allocate more than a small constant number of roots is named captures, where we allocate one string per name. Shrinking these down to 128 bytes instead of 4096 still leaves room for 20+ names without allocating another segment.

While testing this change, I noticed that we had a memory leak. When we allocate named capture information in the parser, we don't have a HandleScope on the stack, so we root the handles in the implicit global handlescope. That scope isn't cleared out during the lifetime of the Isolate. I moved the HandleScope in CompilePattern up to enclose the parser, and added assertions in DestroyIsolate to verify that there aren't any other leaks.

Assignee: nobody → iireland

A couple of notes:

  1. In the previous patch in this stack, I significantly reduced the default size of handleArena and uniquePtrArena (from 4K each down to 128 bytes). We are using SegmentedVector because we need to grow without moving the contents, not because we expect to grow frequently.

  2. I'm reporting the owned memory of the RegExpStack if it has any, but the owned memory is pretty transient. RegExp execution is wrapped in a RegExpStackScope, which should free the owned memory when we're done executing. (That might not always be the case when executing RegExps directly from jitcode, though.)

Depends on D78520

Pushed by
Reduce segment size of root arenas in Isolate r=mgaudet
Add memory reporting to irregexp::Isolate r=mccr8

Backed out 2 changesets (bug 1643171) for jsapi-tests failures

Push with failures:

Backout link:

Failure log:

[task 2020-06-06T00:52:38.326Z] 00:52:38     INFO -  TEST-PASS | testParseJSON_error | ok
[task 2020-06-06T00:52:38.326Z] 00:52:38     INFO -  testParseJSON_success
[task 2020-06-06T00:52:38.326Z] 00:52:38     INFO -  TEST-PASS | testParseJSON_success | ok
[task 2020-06-06T00:52:38.326Z] 00:52:38     INFO -  testObjectEmulatingUndefined_equal
[task 2020-06-06T00:52:38.326Z] 00:52:38     INFO -  TEST-PASS | testObjectEm
[task 2020-06-06T00:52:38.326Z] 00:52:38     INFO -  mozcrash checking c:\users\task_1591402862\appdata\local\temp\tmpbrji8k for minidumps...
[task 2020-06-06T00:52:38.326Z] 00:52:38  WARNING -  TEST-UNEXPECTED-FAIL | jsapi-tests.exe | test failed with return code 3221225477
[task 2020-06-06T00:52:38.326Z] 00:52:38     INFO -  TEST-INFO took 19893ms
[task 2020-06-06T00:52:38.327Z] 00:52:38     INFO -  SUITE-END | took 45s
[task 2020-06-06T00:52:38.327Z] 00:52:38     INFO -  Result summary:
[task 2020-06-06T00:52:38.327Z] 00:52:38     INFO -  cppunittests INFO | Passed: 71
[task 2020-06-06T00:52:38.327Z] 00:52:38  WARNING -  cppunittests INFO | Failed: 1
[task 2020-06-06T00:52:38.327Z] 00:52:38  WARNING -  One or more unittests failed.
[task 2020-06-06T00:52:38.372Z] 00:52:38    ERROR - Return code: 1
[task 2020-06-06T00:52:38.372Z] 00:52:38     INFO - TinderboxPrint: cppunittest-cppunittest<br/>71/<em class="testfail">1</em>
Flags: needinfo?(iireland)
Flags: needinfo?(iireland)
Pushed by
Reduce segment size of root arenas in Isolate r=mgaudet
Pushed by
Add memory reporting to irregexp::Isolate r=mccr8
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
You need to log in before you can comment on or make changes to this bug.