Last Comment Bug 684110 - clean up JSObject::clasp usage and a few other refactorings from bug 683361
: clean up JSObject::clasp usage and a few other refactorings from bug 683361
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-01 17:10 PDT by Luke Wagner [:luke]
Modified: 2011-09-04 21:03 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
clean clasp usage (259.85 KB, patch)
2011-09-01 17:10 PDT, Luke Wagner [:luke]
paul.biggar: review+
Details | Diff | Splinter Review
remove/inline array helper (1.25 KB, patch)
2011-09-01 17:12 PDT, Luke Wagner [:luke]
paul.biggar: review+
Details | Diff | Splinter Review
tidy up the Exception constructor (4.07 KB, patch)
2011-09-01 17:14 PDT, Luke Wagner [:luke]
wmccloskey: review+
Details | Diff | Splinter Review
simplify InitExnPrivate (9.18 KB, patch)
2011-09-01 17:15 PDT, Luke Wagner [:luke]
wmccloskey: review+
Details | Diff | Splinter Review
remove empty-script special case in Invoke (2.50 KB, patch)
2011-09-01 17:16 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
simplify IsActiveWithOrBlock, tidy jsinterp.h (30.82 KB, patch)
2011-09-01 17:22 PDT, Luke Wagner [:luke]
paul.biggar: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-09-01 17:10:42 PDT
Created attachment 557700 [details] [diff] [review]
clean clasp usage

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.
Comment 1 Luke Wagner [:luke] 2011-09-01 17:12:28 PDT
Created attachment 557701 [details] [diff] [review]
remove/inline array helper

No reason for the runaround.
Comment 2 Luke Wagner [:luke] 2011-09-01 17:14:02 PDT
Created attachment 557703 [details] [diff] [review]
tidy up the Exception constructor

Use CallArgs.
Comment 3 Luke Wagner [:luke] 2011-09-01 17:15:38 PDT
Created attachment 557708 [details] [diff] [review]
simplify InitExnPrivate

The two-pass solution was complex and extremely error-prone.
Comment 4 Luke Wagner [:luke] 2011-09-01 17:16:52 PDT
Created attachment 557710 [details] [diff] [review]
remove empty-script special case in Invoke

I'm pretty sure this isn't winning us anything perf-wise.
Comment 5 Luke Wagner [:luke] 2011-09-01 17:22:29 PDT
Created attachment 557716 [details] [diff] [review]
simplify IsActiveWithOrBlock, tidy jsinterp.h

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 { ... }.
Comment 6 Luke Wagner [:luke] 2011-09-01 17:32:53 PDT
Comment on attachment 557700 [details] [diff] [review]
clean clasp usage

Oops, Waldo is hiking.  Paul!
Comment 7 Paul Biggar 2011-09-01 18:08:59 PDT
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()
Comment 8 [PTO to Dec5] Bill McCloskey (:billm) 2011-09-01 18:14:58 PDT
(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.
Comment 9 Paul Biggar 2011-09-01 18:52:37 PDT
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?
Comment 10 Luke Wagner [:luke] 2011-09-02 17:26:37 PDT
(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.
Comment 11 [PTO to Dec5] Bill McCloskey (:billm) 2011-09-02 17:38:21 PDT
Comment on attachment 557708 [details] [diff] [review]
simplify InitExnPrivate

Looks nice. What's the STATIC_ASSERT for?
Comment 12 Luke Wagner [:luke] 2011-09-02 17:41:21 PDT
(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.
Comment 14 Ed Morley [:emorley] 2011-09-02 19:33:26 PDT
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! :-)
Comment 15 [PTO to Dec5] Bill McCloskey (:billm) 2011-09-02 19:40:09 PDT
Sorry, this was my fault.
Comment 18 Luke Wagner [:luke] 2011-09-04 21:03:43 PDT
Oops, I set 'tracking', not 'target milestone'.

Note You need to log in before you can comment on or make changes to this bug.