1643171 - Add memory reporter for regexp Isolate

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Iain Ireland [:iain]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

(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.

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1643171: Reduce segment size of root arenas in Isolate r=mgaudet

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.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1643171: Add memory reporting to irregexp::Isolate r=mccr8

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58363dfc9159
Reduce segment size of root arenas in Isolate r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/094e467aacd7
Add memory reporting to irregexp::Isolate r=mccr8

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=cppunit&fromchange=3f947729cb8a2f83d10ec1ca2501083c33af778c&tochange=42f05268a80bce0e2d7ec3680de596af0f88c2cc&selectedTaskRun=RZroHulsQZi-oXtaDKxbQA-0

Backout link: https://hg.mozilla.org/integration/autoland/rev/42f05268a80bce0e2d7ec3680de596af0f88c2cc

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305295704&repo=autoland&lineNumber=2063

...
[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>

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14b216dc797e
Reduce segment size of root arenas in Isolate r=mgaudet

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d34b4f37cf8a
Add memory reporting to irregexp::Isolate r=mccr8

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/14b216dc797e
https://hg.mozilla.org/mozilla-central/rev/d34b4f37cf8a