Bug 1215430 - Refactor RegExp code to be more spec-like in its ordering of things, and eliminate the confusing statefulness of RegExpObjectBuilder. r=efaust
authorJeff Walden <jwalden@mit.edu>
Fri, 16 Oct 2015 00:30:02 -0700
changeset 304311 bc949d6e3aaab935712bf73430cadb7bc6d7fe26
parent 304310 8009ed0eb3a6ddd265fc236c0478f73130a9f07f
child 304312 6dc0a7e6f79178df535c0c55358cb0c51aa09c10
push id1001
push userraliiev@mozilla.com
push dateMon, 18 Jan 2016 19:06:03 +0000
treeherdermozilla-release@8b89261f3ac4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust
bugs1215430
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1215430 - Refactor RegExp code to be more spec-like in its ordering of things, and eliminate the confusing statefulness of RegExpObjectBuilder. r=efaust
js/src/builtin/RegExp.cpp
js/src/builtin/RegExp.h
js/src/vm/RegExpObject.cpp
js/src/vm/RegExpObject.h
--- a/js/src/builtin/RegExp.cpp
+++ b/js/src/builtin/RegExp.cpp
@@ -135,21 +135,23 @@ js::ExecuteRegExpLegacy(JSContext* cx, R
         /* Forbid an array, as an optimization. */
         rval.setBoolean(true);
         return true;
     }
 
     return CreateRegExpMatchResult(cx, input, matches, rval);
 }
 
-/* ES6 draft rc4 21.2.3.2.2. */
-bool
-RegExpInitialize(JSContext* cx, RegExpObjectBuilder& builder,
-                 HandleValue patternValue, HandleValue flagsValue,
-                 RegExpStaticsUse staticsUse, MutableHandleObject result)
+/*
+ * ES6 21.2.3.2.2.  Because this function only ever returns |obj| in the spec,
+ * provided by the user, we omit it and just return the usual success/failure.
+ */
+static bool
+RegExpInitialize(JSContext* cx, Handle<RegExpObject*> obj, HandleValue patternValue,
+                 HandleValue flagsValue, RegExpStaticsUse staticsUse)
 {
     RootedAtom pattern(cx);
     if (patternValue.isUndefined()) {
         /* Step 1. */
         pattern = cx->runtime()->emptyString;
     } else {
         /* Steps 2-3. */
         pattern = ToAtom<CanGC>(cx, patternValue);
@@ -178,22 +180,20 @@ RegExpInitialize(JSContext* cx, RegExpOb
     if (staticsUse == UseRegExpStatics) {
         RegExpStatics* res = cx->global()->getRegExpStatics(cx);
         if (!res)
             return false;
         flags = RegExpFlag(flags | res->getFlags());
     }
 
     /* Steps 11-15. */
-    RootedObject reobj(cx, builder.build(pattern, flags));
-    if (!reobj)
+    if (!InitializeRegExp(cx, obj, pattern, flags))
         return false;
 
     /* Step 16. */
-    result.set(reobj);
     return true;
 }
 
 MOZ_ALWAYS_INLINE bool
 IsRegExpObject(HandleValue v)
 {
     return v.isObject() && v.toObject().is<RegExpObject>();
 }
@@ -231,17 +231,17 @@ js::IsRegExp(JSContext* cx, HandleValue 
 }
 
 /* ES6 B.2.5.1. */
 MOZ_ALWAYS_INLINE bool
 regexp_compile_impl(JSContext* cx, const CallArgs& args)
 {
     MOZ_ASSERT(IsRegExpObject(args.thisv()));
 
-    RegExpObjectBuilder builder(cx, &args.thisv().toObject().as<RegExpObject>());
+    Rooted<RegExpObject*> regexp(cx, &args.thisv().toObject().as<RegExpObject>());
 
     // Step 3.
     RootedValue patternValue(cx, args.get(0));
     ESClassValue cls;
     if (!GetClassOfValue(cx, patternValue, &cls))
         return false;
     if (cls == ESClass_RegExp) {
         // Step 3a.
@@ -263,34 +263,32 @@ regexp_compile_impl(JSContext* cx, const
             if (!RegExpToShared(cx, patternObj, &g))
                 return false;
 
             sourceAtom = g->getSource();
             flags = g->getFlags();
         }
 
         // Step 5.
-        RegExpObject* reobj = builder.build(sourceAtom, flags);
-        if (!reobj)
+        if (!InitializeRegExp(cx, regexp, sourceAtom, flags))
             return false;
 
-        args.rval().setObject(*reobj);
+        args.rval().setObject(*regexp);
         return true;
     }
 
     // Step 4.
     RootedValue P(cx, patternValue);
     RootedValue F(cx, args.get(1));
 
     // Step 5.
-    RootedObject reobj(cx);
-    if (!RegExpInitialize(cx, builder, P, F, UseRegExpStatics, &reobj))
+    if (!RegExpInitialize(cx, regexp, P, F, UseRegExpStatics))
         return false;
 
-    args.rval().setObject(*reobj);
+    args.rval().setObject(*regexp);
     return true;
 }
 
 static bool
 regexp_compile(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
 
@@ -326,17 +324,16 @@ js::regexp_construct(JSContext* cx, unsi
             // Step 4b.iii.
             if (patternConstructor.isObject() && patternConstructor.toObject() == args.callee()) {
                 args.rval().set(args[0]);
                 return true;
             }
         }
     }
 
-    RegExpObjectBuilder builder(cx);
     RootedValue patternValue(cx, args.get(0));
 
     // Step 5.
     ESClassValue cls;
     if (!GetClassOfValue(cx, patternValue, &cls))
         return false;
     if (cls == ESClass_RegExp) {
         // Beware!  |patternObj| might be a proxy into another compartment, so
@@ -364,23 +361,27 @@ js::regexp_construct(JSContext* cx, unsi
                 RootedString flagStr(cx, ToString<CanGC>(cx, args[1]));
                 if (!flagStr)
                     return false;
                 if (!ParseRegExpFlags(cx, flagStr, &flags))
                     return false;
             }
         }
 
-        // Steps 8-10.
+        // Steps 8-9.
         // XXX Note bug in step 5c, with respect to step 8.
-        RegExpObject* reobj = builder.build(sourceAtom, flags);
-        if (!reobj)
+        Rooted<RegExpObject*> regexp(cx, RegExpAlloc(cx));
+        if (!regexp)
             return false;
 
-        args.rval().setObject(*reobj);
+        // Step 10.
+        if (!InitializeRegExp(cx, regexp, sourceAtom, flags))
+            return false;
+
+        args.rval().setObject(*regexp);
         return true;
     }
 
     RootedValue P(cx);
     RootedValue F(cx);
 
     // Step 6.
     if (patternIsRegExp) {
@@ -397,44 +398,50 @@ js::regexp_construct(JSContext* cx, unsi
                 return false;
         }
     } else {
         // Steps 7a-b.
         P = patternValue;
         F = args.get(1);
     }
 
-    // Steps 8-10.
-    RootedObject reobj(cx);
-    if (!RegExpInitialize(cx, builder, P, F, UseRegExpStatics, &reobj))
+    // Steps 8-9.
+    Rooted<RegExpObject*> regexp(cx, RegExpAlloc(cx));
+    if (!regexp)
         return false;
 
-    args.rval().setObject(*reobj);
+    // Step 10.
+    if (!RegExpInitialize(cx, regexp, P, F, UseRegExpStatics))
+        return false;
+
+    args.rval().setObject(*regexp);
     return true;
 }
 
 bool
 js::regexp_construct_no_statics(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
 
     MOZ_ASSERT(args.length() == 1 || args.length() == 2);
     MOZ_ASSERT(args[0].isString());
     MOZ_ASSERT_IF(args.length() == 2, args[1].isString());
     MOZ_ASSERT(!args.isConstructing());
 
     /* Steps 1-6 are not required since pattern is always string. */
 
     /* Steps 7-10. */
-    RegExpObjectBuilder builder(cx);
-    RootedObject reobj(cx);
-    if (!RegExpInitialize(cx, builder, args[0], args.get(1), DontUseRegExpStatics, &reobj))
+    Rooted<RegExpObject*> regexp(cx, RegExpAlloc(cx));
+    if (!regexp)
         return false;
 
-    args.rval().setObject(*reobj);
+    if (!RegExpInitialize(cx, regexp, args[0], args.get(1), DontUseRegExpStatics))
+        return false;
+
+    args.rval().setObject(*regexp);
     return true;
 }
 
 /* ES6 draft rev32 21.2.5.4. */
 MOZ_ALWAYS_INLINE bool
 regexp_global_impl(JSContext* cx, const CallArgs& args)
 {
     MOZ_ASSERT(IsRegExpObject(args.thisv()));
@@ -688,22 +695,18 @@ js::CreateRegExpPrototype(JSContext* cx,
 {
     MOZ_ASSERT(key == JSProto_RegExp);
 
     Rooted<RegExpObject*> proto(cx, cx->global()->createBlankPrototype<RegExpObject>(cx));
     if (!proto)
         return nullptr;
     proto->NativeObject::setPrivate(nullptr);
 
-    HandlePropertyName empty = cx->names().empty;
-    RegExpObjectBuilder builder(cx, proto);
-    if (!builder.build(empty, RegExpFlag(0)))
-        return nullptr;
-
-    return proto;
+    RootedAtom source(cx, cx->names().empty);
+    return InitializeRegExp(cx, proto, source, RegExpFlag(0));
 }
 
 static bool
 ReportLastIndexNonwritable(JSContext* cx)
 {
     JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_READ_ONLY, "\"lastIndex\"");
     return false;
 }
--- a/js/src/builtin/RegExp.h
+++ b/js/src/builtin/RegExp.h
@@ -91,17 +91,15 @@ extern bool
 regexp_construct_no_statics(JSContext* cx, unsigned argc, Value* vp);
 
 extern bool
 IsRegExp(JSContext* cx, HandleValue value, bool* result);
 
 // RegExp ClassSpec members used in RegExpObject.cpp.
 extern bool
 regexp_construct(JSContext* cx, unsigned argc, Value* vp);
-extern JSObject*
-CreateRegExpPrototype(JSContext* cx, JSProtoKey key);
 extern const JSPropertySpec regexp_static_props[];
 extern const JSPropertySpec regexp_properties[];
 extern const JSFunctionSpec regexp_methods[];
 
 } /* namespace js */
 
 #endif /* builtin_RegExp_h */
--- a/js/src/vm/RegExpObject.cpp
+++ b/js/src/vm/RegExpObject.cpp
@@ -35,106 +35,34 @@ using js::frontend::TokenStream;
 
 using JS::AutoCheckCannotGC;
 
 JS_STATIC_ASSERT(IgnoreCaseFlag == JSREG_FOLD);
 JS_STATIC_ASSERT(GlobalFlag == JSREG_GLOB);
 JS_STATIC_ASSERT(MultilineFlag == JSREG_MULTILINE);
 JS_STATIC_ASSERT(StickyFlag == JSREG_STICKY);
 
-/* RegExpObjectBuilder */
-
-RegExpObjectBuilder::RegExpObjectBuilder(ExclusiveContext* cx, RegExpObject* reobj)
-  : cx(cx), reobj_(cx, reobj)
-{}
-
-bool
-RegExpObjectBuilder::getOrCreate()
+RegExpObject*
+js::RegExpAlloc(ExclusiveContext* cx)
 {
-    if (reobj_)
-        return true;
-
     // Note: RegExp objects are always allocated in the tenured heap. This is
     // not strictly required, but simplifies embedding them in jitcode.
-    reobj_ = NewBuiltinClassInstance<RegExpObject>(cx, TenuredObject);
-    if (!reobj_)
-        return false;
-    reobj_->initPrivate(nullptr);
-
-    return true;
-}
+    RegExpObject* regexp = NewBuiltinClassInstance<RegExpObject>(cx, TenuredObject);
+    if (!regexp)
+        return nullptr;
 
-bool
-RegExpObjectBuilder::getOrCreateClone(HandleObjectGroup group)
-{
-    MOZ_ASSERT(!reobj_);
-    MOZ_ASSERT(group->clasp() == &RegExpObject::class_);
-
-    // Note: RegExp objects are always allocated in the tenured heap. This is
-    // not strictly required, but simplifies embedding them in jitcode.
-    reobj_ = NewObjectWithGroup<RegExpObject>(cx->asJSContext(), group, TenuredObject);
-    if (!reobj_)
-        return false;
-    reobj_->initPrivate(nullptr);
-
-    return true;
+    regexp->initPrivate(nullptr);
+    return regexp;
 }
 
 RegExpObject*
-RegExpObjectBuilder::build(HandleAtom source, RegExpShared& shared)
-{
-    if (!getOrCreate())
-        return nullptr;
-
-    if (!reobj_->init(cx, source, shared.getFlags()))
-        return nullptr;
-
-    reobj_->setShared(shared);
-    return reobj_;
-}
-
-RegExpObject*
-RegExpObjectBuilder::build(HandleAtom source, RegExpFlag flags)
-{
-    if (!getOrCreate())
-        return nullptr;
-
-    return reobj_->init(cx, source, flags) ? reobj_.get() : nullptr;
-}
-
-RegExpObject*
-RegExpObjectBuilder::clone(Handle<RegExpObject*> other)
+js::InitializeRegExp(ExclusiveContext* cx, Handle<RegExpObject*> regexp, HandleAtom source,
+                     RegExpFlag flags)
 {
-    RootedObjectGroup group(cx, other->group());
-    if (!getOrCreateClone(group))
-        return nullptr;
-
-    /*
-     * Check that the RegExpShared for the original is okay to use in
-     * the clone -- if the |RegExpStatics| provides more flags we'll
-     * need a different |RegExpShared|.
-     */
-    RegExpStatics* res = other->getProto()->global().getRegExpStatics(cx);
-    if (!res)
-        return nullptr;
-
-    RegExpFlag origFlags = other->getFlags();
-    RegExpFlag staticsFlags = res->getFlags();
-    if ((origFlags & staticsFlags) != staticsFlags) {
-        RegExpFlag newFlags = RegExpFlag(origFlags | staticsFlags);
-        Rooted<JSAtom*> source(cx, other->getSource());
-        return build(source, newFlags);
-    }
-
-    RegExpGuard g(cx);
-    if (!other->getShared(cx->asJSContext(), &g))
-        return nullptr;
-
-    Rooted<JSAtom*> source(cx, other->getSource());
-    return build(source, *g);
+    return regexp->init(cx, source, flags) ? regexp : nullptr;
 }
 
 /* MatchPairs */
 
 bool
 MatchPairs::initArrayFrom(MatchPairs& copyFrom)
 {
     MOZ_ASSERT(copyFrom.pairCount() > 0);
@@ -287,18 +215,21 @@ RegExpObject::createNoStatics(ExclusiveC
                                    (const char16_t*) nullptr, 0,
                                    (frontend::StrictModeGetter*) nullptr);
         tokenStream = dummyTokenStream.ptr();
     }
 
     if (!irregexp::ParsePatternSyntax(*tokenStream, alloc, source))
         return nullptr;
 
-    RegExpObjectBuilder builder(cx);
-    return builder.build(source, flags);
+    Rooted<RegExpObject*> regexp(cx, RegExpAlloc(cx));
+    if (!regexp)
+        return nullptr;
+
+    return InitializeRegExp(cx, regexp, source, flags);
 }
 
 bool
 RegExpObject::createShared(JSContext* cx, RegExpGuard* g)
 {
     Rooted<RegExpObject*> self(cx, this);
 
     MOZ_ASSERT(!maybeShared());
@@ -940,21 +871,56 @@ RegExpCompartment::sizeOfExcludingThis(m
     return n;
 }
 
 /* Functions */
 
 JSObject*
 js::CloneRegExpObject(JSContext* cx, JSObject* obj_)
 {
-    RegExpObjectBuilder builder(cx);
     Rooted<RegExpObject*> regex(cx, &obj_->as<RegExpObject>());
-    JSObject* res = builder.clone(regex);
-    MOZ_ASSERT_IF(res, res->group() == regex->group());
-    return res;
+
+    // Check that the RegExpShared for |regex| is okay to reuse in the clone.
+    // If the |RegExpStatics| provides additional flags, we'll need a new
+    // |RegExpShared|.
+    RegExpStatics* currentStatics = regex->getProto()->global().getRegExpStatics(cx);
+    if (!currentStatics)
+        return nullptr;
+
+    Rooted<JSAtom*> source(cx, regex->getSource());
+
+    RegExpFlag origFlags = regex->getFlags();
+    RegExpFlag staticsFlags = currentStatics->getFlags();
+    if ((origFlags & staticsFlags) != staticsFlags) {
+        Rooted<RegExpObject*> clone(cx, RegExpAlloc(cx));
+        if (!clone)
+            return nullptr;
+
+        return InitializeRegExp(cx, clone, source, RegExpFlag(origFlags | staticsFlags));
+    }
+
+    // Otherwise, the clone can use |regexp|'s RegExpShared.
+    RootedObjectGroup group(cx, regex->group());
+
+    // Note: RegExp objects are always allocated in the tenured heap. This is
+    // not strictly required, but it simplifies embedding them in jitcode.
+    Rooted<RegExpObject*> clone(cx, NewObjectWithGroup<RegExpObject>(cx, group, TenuredObject));
+    if (!clone)
+        return nullptr;
+    clone->initPrivate(nullptr);
+
+    RegExpGuard g(cx);
+    if (!regex->getShared(cx, &g))
+        return nullptr;
+
+    if (!InitializeRegExp(cx, clone, source, g->getFlags()))
+        return nullptr;
+
+    clone->setShared(*g.re());
+    return clone;
 }
 
 static bool
 HandleRegExpFlag(RegExpFlag flag, RegExpFlag* flags)
 {
     if (*flags & flag)
         return false;
     *flags = RegExpFlag(*flags | flag);
--- a/js/src/vm/RegExpObject.h
+++ b/js/src/vm/RegExpObject.h
@@ -58,38 +58,29 @@ enum RegExpFlag
 
 enum RegExpRunStatus
 {
     RegExpRunStatus_Error,
     RegExpRunStatus_Success,
     RegExpRunStatus_Success_NotFound
 };
 
-class RegExpObjectBuilder
-{
-    ExclusiveContext* cx;
-    Rooted<RegExpObject*> reobj_;
+extern RegExpObject*
+RegExpAlloc(ExclusiveContext* cx);
 
-    bool getOrCreate();
-    bool getOrCreateClone(HandleObjectGroup group);
-
-  public:
-    explicit RegExpObjectBuilder(ExclusiveContext* cx, RegExpObject* reobj = nullptr);
+extern RegExpObject*
+InitializeRegExp(ExclusiveContext* cx, Handle<RegExpObject*> regexp, HandleAtom source,
+                 RegExpFlag flags);
 
-    RegExpObject* reobj() { return reobj_; }
-
-    RegExpObject* build(HandleAtom source, RegExpFlag flags);
-    RegExpObject* build(HandleAtom source, RegExpShared& shared);
+// |regexp| is under-typed because this function's used in the JIT.
+extern JSObject*
+CloneRegExpObject(JSContext* cx, JSObject* regexp);
 
-    /* Perform a VM-internal clone. */
-    RegExpObject* clone(Handle<RegExpObject*> other);
-};
-
-JSObject*
-CloneRegExpObject(JSContext* cx, JSObject* obj);
+extern JSObject*
+CreateRegExpPrototype(JSContext* cx, JSProtoKey key);
 
 /*
  * A RegExpShared is the compiled representation of a regexp. A RegExpShared is
  * potentially pointed to by multiple RegExpObjects. Additionally, C++ code may
  * have pointers to RegExpShareds on the stack. The RegExpShareds are kept in a
  * table so that they can be reused when compiling the same regex string.
  *
  * During a GC, RegExpShared instances are marked and swept like GC things.
@@ -452,17 +443,19 @@ class RegExpObject : public NativeObject
     void setShared(RegExpShared& shared) {
         MOZ_ASSERT(!maybeShared());
         NativeObject::setPrivate(&shared);
     }
 
     static void trace(JSTracer* trc, JSObject* obj);
 
   private:
-    friend class RegExpObjectBuilder;
+    friend RegExpObject*
+    InitializeRegExp(ExclusiveContext* cx, Handle<RegExpObject*> regexp, HandleAtom source,
+                     RegExpFlag flags);
 
     bool init(ExclusiveContext* cx, HandleAtom source, RegExpFlag flags);
 
     /*
      * Precondition: the syntax for |source| has already been validated.
      * Side effect: sets the private field.
      */
     bool createShared(JSContext* cx, RegExpGuard* g);