SpiderMonkey should use either argv throughout, or vp throughout, not a mix

RESOLVED WORKSFORME

Status

()

Core
JavaScript Engine
--
trivial
RESOLVED WORKSFORME
7 years ago
4 years ago

People

(Reporter: jimb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

7 years ago
At present, some SpiderMonkey functions take argv, a pointer to the first argument's slot, and use argv[-1] and argv[-2] to access this and callee, whereas others take vp, a pointer to the callee's slot, and use vp[0] and vp[1] to access callee and this. It's hard to believe this has any significance to performance, and it is unclear.

All internal SpiderMonkey calls should use a single convention; I suggest argv, as that value can be subscripted with meaningful indices (argument indices).

Parts of the public API take vp pointers; those should be left unchanged.
(Reporter)

Comment 1

7 years ago
Dunno. SM sure has a lot of JSNative functions; perhaps most of its weight by line.
We used to have JSNative as JSBool(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) but for perf we've killed that old "slow native" sig. We now use JSBool(JSContext *cx, uintN argc, jsval *vp), which came in as the initially-opt-in "fast natives".

Not sure what this bug wants, but we can't go back to slow natives.

AFAIK indexing by small negative constants has no perf impact.

There is a problem with the new signature, though: it's all too easy to index beyond argc, where with slow natives you could safely do so provided the extra field of the JSFunctionSpec by which the native was defined was big enough.

Static analysis will help, bhackett is on the case.

/be
I agree with the sentiment here, but lw was planning on generally replacing the hardcoded-constant indices approaches with a StackFrame type that actually named members! Not sure if there's a bug for that yet.
(In reply to comment #3)
> I agree with the sentiment here,

Where? This bug is not obviously (from its summary) only about argv[-1] and argv[-2], and anyway as Jim noted those are ancient API creatures. In jsapi.h since fast natives were introduced, JS_CALLEE and JS_THIS{,_OBJECT} have abstracted away from the corresponding vp[0] and vp[1] details.

> but lw was planning on generally replacing the
> hardcoded-constant indices approaches with a StackFrame type that actually
> named members! Not sure if there's a bug for that yet.

Not a StackFrame type, a NativeArgs struct: bug 580337.

/be
(In reply to comment #4)
> (In reply to comment #3) 
> In jsapi.h
> since fast natives were introduced, JS_CALLEE and JS_THIS{,_OBJECT} have
> abstracted away from the corresponding vp[0] and vp[1] details.

Right, so within the engine we still have things like ComputeThisFrom{Vp,Argv}. That's the kind of thing that it'd be nice to get rid of.

> Not a StackFrame type, a NativeArgs struct: bug 580337.

Right, thanks! It's a stack frame overlay, but I suppose we don't want to confuse it with our engine-internal stack frame datatype. Also, where's rval in that thing?

Comment 6

7 years ago
For reference, the arguments overlay struct is already in SM and used for calling js::Invoke:
  http://mxr.mozilla.org/mozilla-central/source/js/src/jsinterp.h#841
Post 4.0 I'd like to change JSNative to use it.

(In reply to comment #5)
> Right, so within the engine we still have things like ComputeThisFrom{Vp,Argv}.
> That's the kind of thing that it'd be nice to get rid of.

Not that its doing much code any good, but:
  http://mxr.mozilla.org/mozilla-central/source/js/src/jsinterp.h#861
(Reporter)

Updated

7 years ago
Summary: SpiderMonkey should use argv throughout, never vp → SpiderMonkey should use either argv throughout, or vp throughout, not a mix
(Reporter)

Comment 7

7 years ago
Anything is fine with me, as long as it's used to reduce needless variation.

Updated

7 years ago
Blocks: 666042
Should we mark this bug as resolved, since Waldo [1] described the current move away from these argc & vp and argv to CallArgs?

[1] http://permalink.gmane.org/gmane.comp.mozilla.devel.jseng/12710
Flags: needinfo?(jimb)
(Reporter)

Comment 9

4 years ago
It seems like the inconsistency that motivated me to file the bug originally is gone.

The vp-based calling convention requires the 'this' value, the callee, and the arguments to be laid out in memory in a particular way, and requires the return value to be stored in the callee slot. What made (makes) no sense to me is to sometimes pass 'vp', and other times 'argv', to refer to such a layout.

In contrast, functions like Invoke, and some JSAPI functions, do take 'argv' values, but those also allow the arguments/callee/this/retval to be arranged in memory in whatever way is convenient for the caller. In such cases it makes no sense to pass a 'vp' value, as the other values can't be inferred from it. If the developer can expect that 'argv' denotes scattered invocation values, then that's real information.

As far as I can tell, in the present code, 'argv' is only used when passing scattered invocation values, and 'vp' is only used when passing stack-layout invocation values. So this seems orderly and helpful to me.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(jimb)
Resolution: --- → FIXED
Resolution: FIXED → WORKSFORME
You need to log in before you can comment on or make changes to this bug.