1345177 - Make RegExpShared into a proper GC thing

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

RegExpShared objects are pointed to by RegExpObjects and have pointers to other GC things.  They have custom marking logic with scary comments about how the mark state is usually correct but sometimes not.  They have their own read barriers which haven't always been quite correct.

We should just make them a GC thing and everything would be a lot simpler.

Comment 1

[Andrew McCreight [:mccr8]]

You should also consider making them a CC thing if they are a GC thing, or we can get weird memory blowups in the CC log.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: reg-exp-shared

Make RegExpShared a GC thing.  Leaves RegExpGuard as an alias for Rooted<RegExpSahred> to be fixed in the next patch.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: reg-exp-shared-guard

Replace references to RegExpGuard, mostly renaming.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8851044 [details] [diff] [review]
reg-exp-shared

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

Wow, really nice! That's a lot of scary one-off code gone.

::: js/src/gc/Heap.h
@@ +108,5 @@
>      ATOM,
>      SYMBOL,
>      JITCODE,
>      SCOPE,
> +    REG_EXP_SHARED,

I would have expected REGEXP_SHARED.

::: js/src/vm/RegExpObject.cpp
@@ +1386,5 @@
> +          if (!HandleRegExpFlag(UnicodeFlag, flagsOut))
> +              return false;
> +          break;
> +      default:
> +          return false;

The indentation on splinter doesn't look right here; the cases are outdented from the switch?

::: js/xpconnect/src/XPCJSContext.cpp
@@ +1943,5 @@
>      ZCREPORT_BYTES(pathPrefix + NS_LITERAL_CSTRING("scopes/malloc-heap"),
>          zStats.scopesMallocHeap,
>          "Arrays of binding names and other binding-related data.");
>  
> +    ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("reg-exp-shareds/gc-heap"),

"regexp-shareds/gc-heap"?

@@ +1947,5 @@
> +    ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("reg-exp-shareds/gc-heap"),
> +        zStats.regExpSharedsGCHeap,
> +        "Shared compiled regexp data.");
> +
> +    ZCREPORT_BYTES(pathPrefix + NS_LITERAL_CSTRING("reg-exp-shareds/malloc-heap"),

"regexp-shareds/malloc-heap"

@@ +2985,5 @@
>      REPORT_BYTES(NS_LITERAL_CSTRING("js-main-runtime-gc-heap-committed/unused/gc-things/jitcode"),
>          KIND_OTHER, rtStats.zTotals.unusedGCThings.jitcode,
>          "Unused jitcode cells within non-empty arenas.");
>  
> +    REPORT_BYTES(NS_LITERAL_CSTRING("js-main-runtime-gc-heap-committed/unused/gc-things/reg-exp-shareds"),

and here

@@ +3040,5 @@
>      MREPORT_BYTES(NS_LITERAL_CSTRING("js-main-runtime-gc-heap-committed/used/gc-things/jitcode"),
>          KIND_OTHER, rtStats.zTotals.jitCodesGCHeap,
>          "Used jitcode cells.");
>  
> +    MREPORT_BYTES(NS_LITERAL_CSTRING("js-main-runtime-gc-heap-committed/used/gc-things/reg-exp-shareds"),

and here

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0380d914ad39
Make RegExpShared a GC thing r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/e66f564d9749
Remove use of RegExpGuard r=sfink

Comment 6

[Jon Coppeard (:jonco)]

Backed out for rooting hazards:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0a094026f7d736e56a5a1ea042fe8e531aabce

Comment 7

[Jon Coppeard (:jonco)]

Attachment: fix-rooting-hazards

Making RegExpShared a GC thing means we need to root it when it's live over a call that can GC.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: fix-finalizer

Also, it really does need a finalizer.

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8851579 [details] [diff] [review]
fix-rooting-hazards

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

Another very good reason for this to be a GC thing.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8851581 [details] [diff] [review]
fix-finalizer

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

Why is the destructor ever used other than via the finalizer? Seems like the cleanup code should be directly in the finalize impl.

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #10)
We need to destroy the tables vector, and this happens automatically if we call the destructor.  Maybe it would be clearer to do this in the finalizer, but you end up calling a destructor manually either way.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: fix-finalizer v2

Updated patch with all destruction handled in finalize.

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8851624 [details] [diff] [review]
fix-finalizer v2

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

::: js/src/vm/RegExpObject.h
@@ +167,5 @@
>          return compilationArray[CompilationIndex(mode, latin1)];
>      }
>  
>    public:
> +    ~RegExpShared() = delete;

If the compiler is happy with that, then good. I was assuming there was some subtle reason why the compiler didn't like having it completely deleted which led to the declare-but-do-not-define hack in the other class. But maybe that's stale.

Comment 14

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #13)
It works locally.  I'll see what try thinks.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aac9899a6646
Make RegExpShared a GC thing r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9625ba329e86
Remove use of RegExpGuard r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fdc31fe714a
Fix RegExpShared rooting hazards now it's a GC thing r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd1e9b734b9
Give RegExpShared a finalizer r=sfink

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9238bcffd438
Fix memory leak reported by valgrind r=me

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aac9899a6646
https://hg.mozilla.org/mozilla-central/rev/9625ba329e86
https://hg.mozilla.org/mozilla-central/rev/9fdc31fe714a
https://hg.mozilla.org/mozilla-central/rev/4fd1e9b734b9
https://hg.mozilla.org/mozilla-central/rev/9238bcffd438