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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(6 files)
(deleted),
patch
|
paul.biggar
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
paul.biggar
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
paul.biggar
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
No reason for the runaround.
Attachment #557701 -
Flags: review?(pbiggar)
Assignee | ||
Comment 3•13 years ago
|
||
The two-pass solution was complex and extremely error-prone.
Attachment #557708 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 4•13 years ago
|
||
I'm pretty sure this isn't winning us anything perf-wise.
Attachment #557710 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #557710 -
Flags: review? → review?(bhackett1024)
Updated•13 years ago
|
Attachment #557701 -
Flags: review?(pbiggar) → review+
Assignee | ||
Comment 5•13 years ago
|
||
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•13 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•13 years ago
|
Attachment #557703 -
Flags: review?(jwalden+bmo) → review?(wmccloskey)
Assignee | ||
Updated•13 years ago
|
Attachment #557716 -
Flags: review?(jwalden+bmo) → review?(pbiggar)
Comment 7•13 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.
Updated•13 years ago
|
Attachment #557710 -
Flags: review?(bhackett1024) → review+
Comment 9•13 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•13 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•13 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•13 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]
Comment 14•13 years ago
|
||
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•13 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]
Updated•13 years ago
|
Target Milestone: --- → mozilla9
Comment 17•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Assignee | ||
Comment 18•13 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.
Description
•