571355 - Move RegExp statics out of the cx

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

JSCrossCompartmentWrappers (coming soon in bug 563099) will not isolate RegExp statics. The right way to do this is per-RegExp-constructor, which is pretty much the same thing as per-global. The implementation should do whatever is fastest to store, since the information for the statics must be frequently stored but it's very, very rarely used.

Comment 1

[Andreas Gal :gal]

I actually tried to do this with a read barrier and just store the last result object somewhere, but it turns out we sometimes don't make a result object, so its all very messy. Lets cc cdleary. He was working in this area recently.

Comment 2

[Brendan Eich [:brendan]]

The original design was dynamic scoping considering only global scopes: function f in window-global w1 calling g in w2 where g matches a regexp would effect w1's cx->regExpStatics, not w2's. Not sure this is required for interop, but IE is the browser to test first, not lesser-market-share-than-us ones ;-).

/be

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

We use cx->regExpStatics pretty heavily in the string match/replace code as well, so we'll probably be introducing some extra cycles. It's not blocking wrappers, is it? Just one of those things we have to get done eventually?

Comment 4

[Brendan Eich [:brendan]]

The internal, i.e., during match or replace, uses of cx->regExpStatics could be cleaned up now to use an in/out parameter (or reference in an existing param), which we'd need in any future that gets rid of cx->regExpStatics. This could be a perf win, not sure.

To handle the w1.f calls w2.g scenario in comment 2, we could move regExpStatics from cx to js::CallStack -- this might remove some js_{SaveAndClear,Restore}RegExpStatics calls. cc'ing Luke and Blake.

/be

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #4)
> To handle the w1.f calls w2.g scenario in comment 2, we could move
> regExpStatics from cx to js::CallStack -- this might remove some
> js_{SaveAndClear,Restore}RegExpStatics calls.

I've already got a patch in the works that avoids save/restore unless the replace lambda *actually* wants to clobber the regexp statics. If it does, I don't think we have a choice but to save/restore.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (In reply to comment #4)
> > To handle the w1.f calls w2.g scenario in comment 2, we could move
> > regExpStatics from cx to js::CallStack -- this might remove some
> > js_{SaveAndClear,Restore}RegExpStatics calls.
> 
> I've already got a patch in the works that avoids save/restore unless the
> replace lambda *actually* wants to clobber the regexp statics. If it does, I
> don't think we have a choice but to save/restore.

Yes, no avoiding that as Andreas found the hard way.

The idea of moving from cx to js::CallStack is that it would make implicit some other explicit save-and-clear/restore pairs. Possibly the pair in XPCSafeJSObjectWrapper.cpp (mrbkap should weigh in).

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> The idea of moving from cx to js::CallStack is that it would make implicit some
> other explicit save-and-clear/restore pairs. Possibly the pair in
> XPCSafeJSObjectWrapper.cpp (mrbkap should weigh in).

Yeah. From my understanding of js::CallStack, this would definitely allow us to remove a save/restore pair (since SJOWs need to push their own CallStack anyway).

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

Should wait until after the Yarr port lands, since that's going to play with the regexp statics code a bit.

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP: Move RegExpStatics to global slot.

Passes trace tests and regexp reftests, but I need to go back in and un-kluge a bunch of things.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Is it okay to change the jsapi to have these regexp-statics mutators take a global explicitly? The current signatures only take a cx and there may be no scope chain currently available for cx -- the old signatures were tightly coupled to the fact that statics lived in cx (whether code was executing or not, regexp statics were available).

For example,

extern JS_PUBLIC_API(void)
JS_SetRegExpInput(JSContext *cx, JSString *input, JSBool multiline);

becomes,

extern JS_PUBLIC_API(void)
JS_SetRegExpInput(JSContext *cx, JSObject *obj, JSString *input, JSBool multiline);

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP: Move RegExpStatics to global slot.

Passes make check, mostly unkluged.

Comment 12

[Brendan Eich [:brendan]]

We're breaking API so adding obj params for the global is fine. Signature change is enough, no need to rename (just noting; didn't see you trying). Name change is sometimes good on own merits, but not required.

/be

Comment 13

[Brendan Eich [:brendan]]

And of course we are breaking with good reasons, a short list. Not random, rip the devs' faces off, we changed this just for some aesthetic reason without any substance, "surprise! you lose" changes ;-).

/be

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Move RegExpStatics to global slot.

Adds another global reserved slot and shoves a js::RegExpStatics-wrapping JSObject in there -- this way, when the global is collected, the js::RegExpStatics instance can release its memory as well.

(Pulls cx->regExpStatics() from the current scope chain, so we modify the statics-related JSAPI to take a global object in a newer patch on this stack.)

FIXME comments and redundant statics lookups are removed in newer patches on this stack, just trying to keep each individual patch readable.

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 2. Move away from mutable ref.

The first patch used a mutable ref in place of a member on cx->regExpStatics() to minimize code thrash, this changes it to a mutable pointer return value.

Since |statics->| was getting too long, I changed it back to |res->| in a bunch of places.

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 3. Consolidate cx->regExpStatics() calls.

Looks up cx->regExpStatics() a bunch less times, since it's like 5 dependent loads in the lookup, and threads it as a second parameter alongside cx.

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 4. Separate js::RegExpStatics from a fixed JSContext.

Removes the JSContext * member from the RegExpStatics because globals can execute in any number of contexts. Moves the cx to a method parameter for most of the RegExpStatics methods.

Also, since JSContext no longer needs to embed RegExpStatics, we were able to kill the jsregexpinlines.h file and move inline implementations to jsregexp.h.

Comment 18

[Andreas Gal :gal]

Comment on attachment 469627 [details] [diff] [review]
Move RegExpStatics to global slot.

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -2899,21 +2899,30 @@ JS_GetObjectId(JSContext *cx, JSObject *
> }
> 
> JS_PUBLIC_API(JSObject *)
> JS_NewGlobalObject(JSContext *cx, JSClass *clasp)
> {
>     CHECK_REQUEST(cx);
>     JS_ASSERT(clasp->flags & JSCLASS_IS_GLOBAL);
>     JSObject *obj = NewNonFunction<WithProto::Given>(cx, Valueify(clasp), NULL, NULL);
>-    if (obj &&
>-        !js_SetReservedSlot(cx, obj, JSRESERVED_GLOBAL_COMPARTMENT,
>-                            PrivateValue(cx->compartment))) {
>-        return false;
>+    if (!(obj &&
>+          js_SetReservedSlot(cx, obj, JSRESERVED_GLOBAL_COMPARTMENT,
>+                             PrivateValue(cx->compartment)))) {
>+        return NULL;

This seems unnecessary and strictly harder to read.

>     }
>+
>+    /* FIXME: comment. */

I concur. Fix it :)

>+    JSObject *res = resc_construct(cx);
>+    if (!(res &&
>+          js_SetReservedSlot(cx, obj, JSRESERVED_GLOBAL_REGEXP_STATICS,
>+                             ObjectValue(*res)))) {
>+        return NULL;
>+    }

I think we don't use this style elsewhere, I would prefer as above (originally) but I won't fight you over it.

>+/*
>+ * RegExpStatics allocates memory -- in order to keep the statics stored
>+ * per-global and not leak, we create a js::Class to wrap the C++ instance and
>+ * provide an appropriate finalizer. We store an instance of that js::Class in
>+ * a global reserved slot.
>+ */
>+

resc_ seems to be a terrible prefix. regexp_statics_ maybe? No need to be brief here.

>+extern Class res_class;

Look at the other classes. This naming convention seems way off.

>+static inline JSObject *
>+resc_construct(JSContext *cx)
>+{
>+    JSObject *obj = NewObject<WithProto::Given>(cx, &res_class, NULL, NULL);
>+    if (!obj)
>+        return NULL;
>+    // FIXME: should be context independent.

?

Looks good. r=me if you explain the last fixme to me. Renamings at your leisure.

Comment 19

[Andreas Gal :gal]

Comment on attachment 469628 [details] [diff] [review]
2. Move away from mutable ref.

Lovely.

Comment 20

[Andreas Gal :gal]

Comment on attachment 469631 [details] [diff] [review]
4. Separate js::RegExpStatics from a fixed JSContext.

This takes care of the FIXME.

Comment 21

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 469631 [details] [diff] [review]
4. Separate js::RegExpStatics from a fixed JSContext.

Canceling review request on this member of the patch stack because it didn't work out in the browser build, schwoops.

Comment 22

[Andreas Gal :gal]

If you hit me again before bkap is in the office you can get another review right away, otherwise tomorrow.

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #18)
> Comment on attachment 469627 [details] [diff] [review]
> Move RegExpStatics to global slot.
> 
> >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
> >--- a/js/src/jsapi.cpp
> >+++ b/js/src/jsapi.cpp
> >@@ -2899,21 +2899,30 @@ JS_GetObjectId(JSContext *cx, JSObject *
> > }
> > 
> > JS_PUBLIC_API(JSObject *)
> > JS_NewGlobalObject(JSContext *cx, JSClass *clasp)
> > {
> >     CHECK_REQUEST(cx);
> >     JS_ASSERT(clasp->flags & JSCLASS_IS_GLOBAL);
> >     JSObject *obj = NewNonFunction<WithProto::Given>(cx, Valueify(clasp), NULL, NULL);
> >-    if (obj &&
> >-        !js_SetReservedSlot(cx, obj, JSRESERVED_GLOBAL_COMPARTMENT,
> >-                            PrivateValue(cx->compartment))) {
> >-        return false;
> >+    if (!(obj &&
> >+          js_SetReservedSlot(cx, obj, JSRESERVED_GLOBAL_COMPARTMENT,
> >+                             PrivateValue(cx->compartment)))) {
> >+        return NULL;
> 
> This seems unnecessary and strictly harder to read.

This is not equivalent by De Morgan's theorem, but presumably the very next line returns obj, so it doesn't matter. Agred on style nit, see below for more.

> >+    JSObject *res = resc_construct(cx);
> >+    if (!(res &&
> >+          js_SetReservedSlot(cx, obj, JSRESERVED_GLOBAL_REGEXP_STATICS,
> >+                             ObjectValue(*res)))) {
> >+        return NULL;
> >+    }
> 
> I think we don't use this style elsewhere, I would prefer as above (originally)
> but I won't fight you over it.

We use !res || !fallible_thing_consuming_res(...) in general. Or two ifs since the same number of lines is consumed and you can often avoid bracing (100 char limit on line length, gross including \n!).

> >+/*
> >+ * RegExpStatics allocates memory -- in order to keep the statics stored
> >+ * per-global and not leak, we create a js::Class to wrap the C++ instance and
> >+ * provide an appropriate finalizer. We store an instance of that js::Class in
> >+ * a global reserved slot.
> >+ */
> >+
> 
> resc_ seems to be a terrible prefix. regexp_statics_ maybe? No need to be brief
> here.

$ grep regexp_static *.c
jsregexp.c:enum regexp_static_tinyid {
jsregexp.c:regexp_static_getProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
jsregexp.c:regexp_static_setProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
jsregexp.c:static JSPropertySpec regexp_static_props[] = {
jsregexp.c:     regexp_static_getProperty,    regexp_static_setProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_setProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:     regexp_static_getProperty,    regexp_static_getProperty},
jsregexp.c:                         regexp_static_props, NULL);

from my old Fx3-era sources.

Great to see the explicit obj parameter -- now the trick is getting that right!

/be

Comment 24

[Chris Leary [:cdleary] (not checking bugmail)]

Okay, it wasn't so bad getting it to work in the browser. mq is hosted at http://hg.mozilla.org/users/cleary_mozilla.com/tm-res-global-mq/ -- needs cleanup and to re-incorporate the higher-numbered patches in the above set. Will update the attachments when things are good to go again.

Comment 25

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 0. Add a RegExpStatics-less regular expression interface.

This is the one part that's meaningfully different. I exposed this through the JSAPI for use by things like nsHTMLInputElement -- maybe it would be better off as a friend API, but I figured it could be useful in cases where you you have a context but no global object to speak of.

Once this is reviewed I'll post a folded patch before landing. It passes jsreftests on my box, will push to try and see how the mochitests fare.

Comment 26

[Andreas Gal :gal]

Comment on attachment 473228 [details] [diff] [review]
0. Add a RegExpStatics-less regular expression interface.

Looks good. Can you make sure Brendan is ok with the API change?

Comment 27

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #26)
> Can you make sure Brendan is ok with the API change?

We chatted with Brendan IRL and he seems okay with it.

Comment 28

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Folded patch.

Running the try gamut, then it gets pushed to TM.

Comment 29

[Chris Leary [:cdleary] (not checking bugmail)]

Bam. http://hg.mozilla.org/tracemonkey/rev/552bb56871e0

Comment 30

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/552bb56871e0

Comment 31

[Martin Stránský [:stransky] (ni? me)]

Hey, it has been already 6 months in repo and 

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Reference/JS_NewRegExpObject

is still missing proper documentation. Would you mind to update it? It's really difficult to port apps to new JS w/o it.

Comment 32

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #31)

Updated. Let me know if anything needs further clarification.