1968172 - ObjectToSource rooted vector on the stack causes minor GC to go quadratic (Was: Testcase doing async init on a module spends 3.5 minutes in MinorGC and then errors out. Chrome errors out instantly.)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Mayank Bansal]

Attachment: Testcase.HTML

Open testcase

AR: Firefox takes 3.5 minutes in minorGC and then throws an error. Chrome throws an error instantly.

Firefox profile: https://share.firefox.dev/3HneU1x

**Firefox error **:Uncaught (in promise) TypeError: ({HEAP8:{0:0, 1:0, 2:0, 3:0, 4:0, 5:0, 6:0, 7:0, 8:0, 9:0, 10:0, 11:0, 12:0, 13:0, 14:0, 15:0, 16:0, 17:0, 18:0, 19:0, 20:0, 21:0, 22:0, 23:0, 24:0, 25:0, 26:0, 27:0, 28:0, 29:0, 30:0, 31:0, 32:0, 33:0, 34:0, 35:0, 36:0, 37:0, 38:0, 39:0, 40:0, 41:0, 42:0, 43:0, 44:0, 45:0, 46:0, 47:0, 48:0, 49:0, 50:0, 51:0, 52:0, 53:0, 54:0, 55:0, 56:0, 57:0, 58:0, 59:0, 60:0, 61:0, 62:0, 63:0, 64:0, 65:0, 66:0, 67:0, 68:0, 69:0, 70:0, 71:0, 72:0, 73:0, 74:0, 75:0, 76:0, 77:0, 78:0, 79:0, 80:0, 81:0, 82:0, 83:0, 84:0, 85:0, 86:0, 87:0, 88:0, 89:0, 90:0, 91:0, 92:0, 93:0, 94:0, 95:0, 96:0, 97:0, 98:0, 99:0, 100:0, 101:0, 102:0, 103:0, 104:0, 105:0, 106:0, 107:0, 108:0, 109:0, 110:0, 111:0, 112:0, 113:0, 114:0, 115:0, 116:0, 117:0, 118:0, 119:0, 120:0, 121:0, 122:0, 123:0, 124:0, 125:0, 126:0, 127:0, 128:0, 129:0, 130:0, 131:0, 132:0, 133:0, 134:0, 135:0, 136:0, 137:0, 138:0, 139:0, 140:0, 141:0, 142:0, 143:0, 144:0, 145:0, 146:0, 147:0, 148:0, 149:0, 150:0, 151:0, 152:0, 153:0, 154:0, 155:0, 156:0, 15…

**Chrome error **:

Uncaught (in promise) TypeError: initBinaryen is not a function
    at run (Testcase.HTML:14:30)
    at Testcase.HTML:32:5

Comment 1

[Jon Coppeard (:jonco)]

This looks like another case where having a rooted vector on the stack causes minor GC to go quadratic (probably the one in ObjectToSource).

If someone wants to look into this the fix should be similar to other instances such as bug 1867453, bug 1917397 or bug 1853305.

Comment 2

[Mayank Bansal]

Still repros: https://share.firefox.dev/4oEqOFn
This is a known quadratic behaviour with a testcase. Worth biting the bullet and just fixing it now?

Comment 3

[Jon Coppeard (:jonco)]

This turned out to be a bit more complicated than I realised.

Our implementation of ToSource can create a ton of intermediate strings and this can trigger GC . It would be much more efficient to use a string buffer to build up the final representation instead of creating JSStrings as it went along. This is fixable but a reasonable amount of work.

Another issue is that we don't limit the string representation to some fixed size so converting a large object graph can be very expensive.

Object.prototype.toSource is not directly exposed to the web any more so this is not a widely used feature. This use comes about because we decompile values when generating errors in a bunch of places: https://searchfox.org/firefox-main/search?q=symbol:_ZN2js16ReportValueErrorEP9JSContextjiN2JS6HandleINS2_5ValueEEENS3_IP8JSStringEEPKcSA_&redirect=false

It's probably not worth it to rewrite ToSource to fix this.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1968172 - Skip tracing rooted vectors of non-nursery types in minor GC r?jandem

I noticed that for this test case the vector we're tracing repeatedly in
minor GC doesn't ever actually contain any nursery pointers.

The patch adds a base class to the GCPolicy template to add a default
implementation of mightBeInNursery(), returning true. A definition for
GCPolicy<jsid> overrides this since Ids can't be in the nursery. Finally we can
check this in StackGCVector::trace to skip tracing during minor GC for
RootedVectors.

This mostly removes the cost of nursery collection for the test case.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d33b4c513b87
https://hg.mozilla.org/integration/autoland/rev/1493035f6cab
Skip tracing rooted vectors of non-nursery types in minor GC r=jandem

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1493035f6cab

Comment 7

[Mayank Bansal]

the MinorGC parts are all gone: https://share.firefox.dev/47Yu8oG

Comment 8

[Mayank Bansal]

The patch description sounds like the fix is more general and not specific to this testcase/usecase. If that is correct, where else could improvements be seen?

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Mayank Bansal from comment #8)
Anywhere you have a rooted vector of jsid/Id/PropertyKey on the stack and it can be large, e.g. populated by the keys of an array or object with many properties. I feel like we would have noticed if there were other problematic instances of this (we have previously fixed a few instances of similar problems) but you never know.