Make RegExpShared into a proper GC thing

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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)
(Assignee)

Comment 3

2 years ago
Replace references to RegExpGuard, mostly renaming.
Attachment #8851045 - Flags: review?(sphink)
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
Attachment #8851044 - Flags: review?(sphink) → review+
Attachment #8851045 - Flags: review?(sphink) → review+
(Assignee)

Comment 7

2 years ago
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)
(Assignee)

Comment 8

2 years ago
Posted 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]
fix-rooting-hazards

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]
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.
(Assignee)

Comment 11

2 years ago
(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.
(Assignee)

Comment 12

2 years ago
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+
(Assignee)

Comment 14

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

Comment 16

2 years ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9238bcffd438
Fix memory leak reported by valgrind r=me
(Assignee)

Updated

2 years ago
Depends on: 1355050
You need to log in before you can comment on or make changes to this bug.