The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Assignee)

Description

6 years ago
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.
Attachment #557700 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 1

6 years ago
Created attachment 557701 [details] [diff] [review]
remove/inline array helper

No reason for the runaround.
Attachment #557701 - Flags: review?(pbiggar)
(Assignee)

Comment 2

6 years ago
Created attachment 557703 [details] [diff] [review]
tidy up the Exception constructor

Use CallArgs.
Attachment #557703 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

6 years ago
Created attachment 557708 [details] [diff] [review]
simplify InitExnPrivate

The two-pass solution was complex and extremely error-prone.
Attachment #557708 - Flags: review?(wmccloskey)
(Assignee)

Comment 4

6 years ago
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.
Attachment #557710 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #557710 - Flags: review? → review?(bhackett1024)

Updated

6 years ago
Attachment #557701 - Flags: review?(pbiggar) → review+
(Assignee)

Comment 5

6 years ago
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 { ... }.
Attachment #557716 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

6 years ago
Comment on attachment 557700 [details] [diff] [review]
clean clasp usage

Oops, Waldo is hiking.  Paul!
Attachment #557700 - Flags: review?(jwalden+bmo) → review?(pbiggar)
(Assignee)

Updated

6 years ago
Attachment #557703 - Flags: review?(jwalden+bmo) → review?(wmccloskey)
(Assignee)

Updated

6 years ago
Attachment #557716 - Flags: review?(jwalden+bmo) → review?(pbiggar)

Comment 7

6 years ago
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 9

6 years ago
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+
(Assignee)

Comment 10

6 years ago
(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+
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/08b6eaf6aad0
http://hg.mozilla.org/integration/mozilla-inbound/rev/cf8b35fa1010
http://hg.mozilla.org/integration/mozilla-inbound/rev/25ee45edabe1
http://hg.mozilla.org/integration/mozilla-inbound/rev/659f5c7d2cc9
http://hg.mozilla.org/integration/mozilla-inbound/rev/870f6dd82586
http://hg.mozilla.org/integration/mozilla-inbound/rev/e1ad65d6a7fd
Whiteboard: [inbound]
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]
Sorry, this was my fault.
(Assignee)

Comment 16

6 years ago
np bill

http://hg.mozilla.org/integration/mozilla-inbound/rev/e6e99374aae9
http://hg.mozilla.org/integration/mozilla-inbound/rev/52e5861882de
http://hg.mozilla.org/integration/mozilla-inbound/rev/17af4431bb45
http://hg.mozilla.org/integration/mozilla-inbound/rev/b750fbde4ca9
http://hg.mozilla.org/integration/mozilla-inbound/rev/71c08d14b557
http://hg.mozilla.org/integration/mozilla-inbound/rev/ecdad0ca5b00
tracking-firefox9: --- → +
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/ecdad0ca5b00
http://hg.mozilla.org/mozilla-central/rev/71c08d14b557
http://hg.mozilla.org/mozilla-central/rev/b750fbde4ca9
http://hg.mozilla.org/mozilla-central/rev/17af4431bb45
http://hg.mozilla.org/mozilla-central/rev/52e5861882de
http://hg.mozilla.org/mozilla-central/rev/e6e99374aae9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
(Assignee)

Comment 18

6 years ago
Oops, I set 'tracking', not 'target milestone'.
tracking-firefox9: + → ---
You need to log in before you can comment on or make changes to this bug.