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 269163 bc949d6e3aaab935712bf73430cadb7bc6d7fe26
parent 269162 8009ed0eb3a6ddd265fc236c0478f73130a9f07f
child 269164 6dc0a7e6f79178df535c0c55358cb0c51aa09c10
push id15832
push usercbook@mozilla.com
push dateFri, 23 Oct 2015 09:51:35 +0000
treeherderfx-team@308fb512df21 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust
bugs1215430
milestone44.0a1
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);