Last Comment Bug 724748 - simplify RegExpShared lifetime management
: simplify RegExpShared lifetime management
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: mozilla13
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on: 726380 731181 763372
Blocks: 673274 723892
  Show dependency treegraph
Reported: 2012-02-06 19:08 PST by Luke Wagner [:luke]
Modified: 2012-06-10 19:53 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (106.70 KB, patch)
2012-02-06 19:08 PST, Luke Wagner [:luke]
cdleary: review+
Details | Diff | Splinter Review

Description User image Luke Wagner [:luke] 2012-02-06 19:08:28 PST
Created attachment 594883 [details] [diff] [review]

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 User image Luke Wagner [:luke] 2012-02-06 19:10:10 PST
Oops, ignore "(The patch removes a".
Comment 2 User image Chris Leary [:cdleary] (not checking bugmail) 2012-02-10 16:29:59 PST
Comment on attachment 594883 [details] [diff] [review]

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 4 User image Ed Morley [:emorley] 2012-02-11 03:54:31 PST
Looking good :-)!topic/
Comment 5 User image Luke Wagner [:luke] 2012-02-11 10:34:03 PST
Whoa, sweet!  Thanks for pointing that out.
Comment 6 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2012-03-12 16:45:37 PDT
Looks like this landed on m-c a month ago without it being noted here:

Given that, can this bug be RESOLVED|FIXED?

Note You need to log in before you can comment on or make changes to this bug.