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)
259.85 KB,
patch
|
paul.biggar
:
review+
|
Details | Diff | Splinter Review |
1.25 KB,
patch
|
paul.biggar
:
review+
|
Details | Diff | Splinter Review |
4.07 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
9.18 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
30.82 KB,
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
•