Closed Bug 724748 Opened 13 years ago Closed 13 years ago

simplify RegExpShared lifetime management

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file)

Attached patch patch (deleted) — Splinter 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.
Attachment #594883 - Flags: review?(christopher.leary)
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+
Target Milestone: --- → mozilla13
Whoa, sweet! Thanks for pointing that out.
Depends on: 726380
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
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 731181
Depends on: 763372
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: