724748 - simplify RegExpShared lifetime management

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

Currently RegExpShared is ref-counted even though all RegExpShareds are discarded on every GC.  This patch creates a RegExpCompartment that owns all RegExpShareds in the compartment and deletes them all on every GC.  The special case is when a RegExpShared is being used (to execute a regexp) during GC.  For this, a simple stack-based counter is used and RegExpShareds with a non-zero use count are allowed to live.  All pointers from RegExpObjects to RegExpShareds are cleared on every GC.

It could be a coincidence, but the patch shows a distinct 1.5% improvement on the v8-regexp score.  (The patch removes a 

The patch also defragments the code, pulling stuff together in the .cpp and out of the -inl.h to make it more readable.  Also, the "Matcher" abstraction is removed, replaced by direct RegExpShared usage and a simple RegExpShared::Guard.

One last thing: it seems that the greedy dot-star optimization (bug 691797) was never actually active.  I discovered this when (having inadvertently fixed the bug which kept the optimization off) I broke a regexp statics test added by bug 691797.  Fixing the test seems non-trivial (bug 691797 comment 6 mentions what v8 does) so I just made StartsWithGreedyStar always return 'false' so this can be a followup.

Comment 1

[Luke Wagner [:luke]]

Oops, ignore "(The patch removes a".

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 594883 [details] [diff] [review]
patch

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

Yay, betterness.

::: js/src/jsstr.cpp
@@ +1351,5 @@
>      int32_t match() const { return match_; }
>  };
>  
> +static inline bool
> +IsRegExpMetaChar(jschar c)

Thanks for moving these inlines. I'll close out my laziness bug 692750.

@@ +1396,5 @@
>      static const size_t MAX_FLAT_PAT_LEN = 256;
>  
> +    static JSAtom *
> +    flattenPattern(JSContext *cx, JSAtom *patstr)
> +    {

Newline for inline function definitions? Gasp!

::: js/src/methodjit/Compiler.cpp
@@ +6943,3 @@
>       * so that we grab it in the getNewObject template copy. Note that
>       * JIT code is discarded on every GC, which permits us to burn in
> +     * the pointer to the RegExpShared refcount.

There's no more RegExpShared refcount, so we can probably remove that sentence.

::: js/src/vm/RegExpObject.h
@@ +359,3 @@
>  };
>  
> +class RegExpCompartment

I get it. Like a JaegerCompartment.

@@ +399,5 @@
> +    /*
> +     * A 'hacked' RegExpShared is one where the input 'source' doesn't match
> +     * what is actually compiled in the regexp. To compile a hacked regexp,
> +     * getHack may be called providing both the original 'source' and the
> +     * 'hackedSource' which should actually be compiled. For a given 'source'

Nice generalization.

Comment 3

[Luke Wagner [:luke]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2b630873c4da

Comment 4

[Ed Morley [:emorley]]

Looking good :-)
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/PG12hBLBY6I

Comment 5

[Luke Wagner [:luke]]

Whoa, sweet!  Thanks for pointing that out.

Comment 6

[Daniel Holbert [:dholbert]]

Looks like this landed on m-c a month ago without it being noted here:
 https://hg.mozilla.org/mozilla-central/rev/2b630873c4da

Given that, can this bug be RESOLVED|FIXED?