Closed Bug 1345177 Opened 4 years ago Closed 4 years ago

Make RegExpShared into a proper GC thing


(Core :: JavaScript: GC, enhancement)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: jonco, Assigned: jonco)




(4 files, 1 obsolete file)

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.
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.
Attached patch reg-exp-sharedSplinter Review
Make RegExpShared a GC thing.  Leaves RegExpGuard as an alias for Rooted<RegExpSahred> to be fixed in the next patch.
Assignee: nobody → jcoppeard
Attachment #8851044 - Flags: review?(sphink)
Replace references to RegExpGuard, mostly renaming.
Attachment #8851045 - Flags: review?(sphink)
Comment on attachment 8851044 [details] [diff] [review]

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,

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"),


@@ +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"),


@@ +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
Attachment #8851044 - Flags: review?(sphink) → review+
Attachment #8851045 - Flags: review?(sphink) → review+
Making RegExpShared a GC thing means we need to root it when it's live over a call that can GC.
Attachment #8851579 - Flags: review?(sphink)
Attached patch fix-finalizer (obsolete) — Splinter Review
Also, it really does need a finalizer.
Attachment #8851581 - Flags: review?(sphink)
Comment on attachment 8851579 [details] [diff] [review]

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

Another very good reason for this to be a GC thing.
Attachment #8851579 - Flags: review?(sphink) → review+
Comment on attachment 8851581 [details] [diff] [review]

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.
(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.
Attached patch fix-finalizer v2Splinter Review
Updated patch with all destruction handled in finalize.
Attachment #8851581 - Attachment is obsolete: true
Attachment #8851581 - Flags: review?(sphink)
Attachment #8851624 - Flags: review?(sphink)
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.
Attachment #8851624 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #13)
It works locally.  I'll see what try thinks.
Pushed by
Fix memory leak reported by valgrind r=me
Depends on: 1355050
You need to log in before you can comment on or make changes to this bug.