Closed Bug 684110 Opened 13 years ago Closed 13 years ago

clean up JSObject::clasp usage and a few other refactorings from bug 683361

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(6 files)

Working on bug 683361 I made a few cleanups that I put in separate patches that can be easily reviewed and landed early.

The only big patch consolidates all the JSObject::isX() inline definitions in jsobj.h.  This avoids the annoying "which inlines.h file do I need to #include to make this warning go away" situation that keeps coming up.  Just #include jsobj.h.  This means also consolidating all the js::Class declarations in jsobj.h, which I think makes sense.  Everything else specific to specific object classes should eventually be extracted into vm/XObject{.cpp,.h,-inl.h} modules.

Other patches are simple and will be attached next.
Attachment #557700 - Flags: review?(jwalden+bmo)
No reason for the runaround.
Attachment #557701 - Flags: review?(pbiggar)
Use CallArgs.
Attachment #557703 - Flags: review?(jwalden+bmo)
The two-pass solution was complex and extremely error-prone.
Attachment #557708 - Flags: review?(wmccloskey)
I'm pretty sure this isn't winning us anything perf-wise.
Attachment #557710 - Flags: review?
Attachment #557710 - Flags: review? → review?(bhackett1024)
Attachment #557701 - Flags: review?(pbiggar) → review+
Remove last js_* names in jsinterp.h and take out some decls that were only extern b/c of JS_LONE_INTERPRET (gone).  Lastly, dvander set the precedent that I think is right of naming non-member functions js:: in .cpps instead of wrapping the whole .cpp in namespace js { ... }.
Attachment #557716 - Flags: review?(jwalden+bmo)
Comment on attachment 557700 [details] [diff] [review]
clean clasp usage

Oops, Waldo is hiking.  Paul!
Attachment #557700 - Flags: review?(jwalden+bmo) → review?(pbiggar)
Attachment #557703 - Flags: review?(jwalden+bmo) → review?(wmccloskey)
Attachment #557716 - Flags: review?(jwalden+bmo) → review?(pbiggar)
Comment on attachment 557700 [details] [diff] [review]
clean clasp usage

Review of attachment 557700 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely patch!

There are quite a few clasp == &X_class left, which I guess your regex missed, and they often allow us to remove some code higher up.

::: js/src/jsgcmark.cpp
@@ +707,5 @@
>       * which are not dense arrays (dense arrays have trace hooks).
>       */
>      Class *clasp = obj->getClass();
>      if (clasp->trace) {
> +        if (clasp == &ArrayClass) {

isDenseArray()

::: js/src/jsobj.h
@@ +327,5 @@
> +extern Class StrictArgumentsObjectClass;
> +extern Class WeakMapClass;
> +extern Class WithClass;
> +extern Class XMLFilterClass;
> +

Ahhhh, I feel suddenly calm and serene.

::: js/src/json.cpp
@@ +358,1 @@
>              *vp = obj->getPrimitiveThis();

isX()

::: js/src/jsstr.cpp
@@ +417,1 @@
>              ClassMethodIsNative(cx, obj,

isX()

::: js/src/shell/jsheaptools.cpp
@@ +376,1 @@
>                  return JSVAL_VOID;

This could be obj->isBlock() || ...

::: js/src/vm/Debugger.cpp
@@ +353,1 @@
>      JSObject *dbgobj = &obj->getReservedSlot(JSSLOT_DEBUGOBJECT_OWNER).toObject();

Can we make this isDebuggerFrame(), isDebuggerObject and isDebuggerScript()?

@@ +527,1 @@
>              JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_EXPECTED_TYPE,

isDebuggerObject()

@@ +1483,1 @@
>              Value rv = v;

isDebuggerClass()

@@ +1981,1 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO,

isDebuggerScript()

@@ +3044,1 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO,

isDebuggerObject()

::: js/src/vm/GlobalObject.cpp
@@ +304,1 @@
>      return (DebuggerVector *) debuggers.toObject().getPrivate();

I don't see a isGlobalDebuggees() function in jsobj.h. Intentional?

::: js/src/vm/Stack-inl.h
@@ +362,1 @@
>          JS_ASSERT(IsCacheableNonGlobalScope(pobj) || pobj->isWith());

isCall()
Attachment #557700 - Flags: review?(pbiggar) → review+
(In reply to Paul Biggar from comment #7)
> ::: js/src/jsgcmark.cpp
> @@ +707,5 @@
> >       * which are not dense arrays (dense arrays have trace hooks).
> >       */
> >      Class *clasp = obj->getClass();
> >      if (clasp->trace) {
> > +        if (clasp == &ArrayClass) {
> 
> isDenseArray()

In case Brian doesn't see this, he told me he did this on purpose to avoid the extra load. We're not sure how important this is, but it is a pretty hot path.
Attachment #557710 - Flags: review?(bhackett1024) → review+
Comment on attachment 557716 [details] [diff] [review]
simplify IsActiveWithOrBlock, tidy jsinterp.h

Review of attachment 557716 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinterp.cpp
@@ +1329,5 @@
>      return JS_TRUE;
>  }
>  
> +static void
> +LeaveWith(JSContext *cx)

These lost their static analysis annotations. Intentional?
Attachment #557716 - Flags: review?(pbiggar) → review+
(In reply to Bill McCloskey (:billm) from comment #8)
> In case Brian doesn't see this, he told me he did this on purpose to avoid
> the extra load. We're not sure how important this is, but it is a pretty hot
> path.

To be safe I intentionally left some of those the same.

(In reply to Paul Biggar from comment #9)
> These lost their static analysis annotations. Intentional?

Those are only for the tracer and I don't think that analysis is coming back before the tracer is removed.
Attachment #557703 - Flags: review?(wmccloskey) → review+
Comment on attachment 557708 [details] [diff] [review]
simplify InitExnPrivate

Looks nice. What's the STATIC_ASSERT for?
Attachment #557708 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #11)
> Looks nice. What's the STATIC_ASSERT for?

Let's say that JSStackTraceElem was way bigger than StackFrame, then my reasoning for why there can't be an overflow is invalid.
Backed out of inbound, since either this or bug 684344, caused bustage:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=cd1957f6628d
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=e1ad65d6a7fd

However, no way to tell which since wmccloskey@mozilla.com pushed the next changeset within the 3 minute cut-off for including in the same build cycle :-( (see bug 684436 for stopping doing this, since it's unhelpful at best).

Also, please may you set the target milestone when landing on m-i, per the recently updated https://wiki.mozilla.org/Inbound_Sheriff_Duty

Thanks! :-)
Status: NEW → ASSIGNED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Oops, I set 'tracking', not 'target milestone'.
You need to log in before you can comment on or make changes to this bug.