simplify RegExpShared lifetime management

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 594883 [details] [diff] [review]
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.
Attachment #594883 - Flags: review?(christopher.leary)
(Assignee)

Comment 1

5 years ago
Oops, ignore "(The patch removes a".
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.
Attachment #594883 - Flags: review?(christopher.leary) → review+
(Assignee)

Comment 3

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2b630873c4da
Target Milestone: --- → mozilla13
Looking good :-)
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/PG12hBLBY6I
(Assignee)

Comment 5

5 years ago
Whoa, sweet!  Thanks for pointing that out.

Updated

5 years ago
Depends on: 726380
Blocks: 673274
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?
Version: unspecified → Trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 731181
Blocks: 723892

Updated

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