412296 - Consider removal of minargs support for fast natives

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

[This is a spin-off of bug 412068 comment 22 and 23.]

The minarg argument in JS_FN macro allows to specify the minimal amount of arguments that the fast method caller must supply with each call. It allows to have less checks when implementing native methods when the method treats the missing parameter as an undefined value.

Unfortunately such support leads to bugs like bug 412068 and requires an extra check in the interpreter when calling a fast method even in the cases when minarg == 0. Thus the idea is to remove the support for minarg and add explicit checks for argc and, when necessary, extra local roots.

Comment 1

[Brendan Eich [:brendan]]

Don't want more bugs like that one; don't want this API escaping into a JS1.8 spidermonkey release and being de-facto frozen. So wanted in mozilla1.9.

/be

Comment 2

[Jeff Walden [:Waldo]]

Attachment: WIP

The proportion seems to be 60-40 has-minargs-doesn't, going from memory.  You cannot imagine just how much of a bloody pain it is disentangling the ones that have minargs.  Changes are minimal -- a ternary check here or there usually -- but occasionally tedious to determine.

I must be at least 80% done by now (changed JS_FN, then have been doing the O(n**2) change-a-method-and-its-JSFN a few times then recompile dance); I have tests for the changes up to jsobj.c (when I became incapable of writing any more no-args tests without going insane first).  I am incapable of finishing this work at the moment, so I'm dumping what I have in Bugzilla for backup purposes and in case some masochistic person feels like finishing it before I recuperate.

Sadly, the tests for this can't really be optional: we need them to demonstrate memory safety, and we very likely need to valgrind them (depending on how lucky you can get dereferencing the jsval one past the end of an array).

This doesn't compile, so I have no idea whether or how much it's a win, speedwise.  It's only complex when you're trying to change a couple hundred methods at once.

Comment 3

[Jeff Walden [:Waldo]]

We need to see whether removing minargs is a win before we do an actual release -- don't want to let what might be a bad idea become enshrined in the API.  No idea yet how it affects the bottom line, tho; need to finish the patch first, in my copious spare time.

Comment 4

[Igor Bukanov]

fixing this would simplify fixing the bug 448828. 

Currently, due to the need to push minargs during a fast native call, a value of the stack pointer for caller's interpreter frame cannot be inferred statically based purely on the bytecode structure.

Comment 5

[Igor Bukanov]

Attachment: something that just compiles

Comment 6

[Igor Bukanov]

Attachment: something that just compiles for real

Comment 7

[Igor Bukanov]

Attachment: v1

The new patch passes the tests in the shell. In the previous one I made wrong refactoring of NamespaceHelper/QNameHelper: when the functions are called directly from xml_ method, then they should treat empty argument list to mean the same thing as the single undefined, which is different from the rules for the case when the functions are called to implement the constructors.

Comment 8

[Igor Bukanov]

Attachment: v1 for real

Here is the right patch.

Comment 9

[Igor Bukanov]

Attachment: v2

The new version of the patch improves missing argument reporting. Now with the patch:

@watson~> ~/m/31-ff/js/src/Linux_All_DBG.OBJ/js -e 'Object.getPrototypeOf()'
-e:1: TypeError: missing argument 0 when calling function Object.getPrototypeOf

Without the patch that gives:
@watson~> ~/m/31-ff/js/src/Linux_All_OPT.OBJ/js -e 'Object.getPrototypeOf()'
-e:1: TypeError: (void 0) is undefined

General comments for the patch:

The patch avoided adding explicit tvr-rooting. It was possible to use either vp[0] to root the values before assigning vp[0] with the result of the call or use values like JSVAL_VOID or ATOM_TO_STRING(cx->runtime->atomState.typeAtoms[JSTYPE_NUMBER]) when the code needed to access the arguments beyond the argc. As always, jsxml.c required most of the work to ensure the proper rooting due to convoluted semantics of E4X methods.

To make things simpler in jsstr.c and avoid any performance regressions while minimizing the code bloat the patch introduced ArgToRootedString. This helper function inlines js_ValueToString to avoid unnecessary argv[i] = str when str is already in argv[i] or permanently rooted by the runtime.

Comment 10

[Brian Crowder]

Comment on attachment 332704 [details] [diff] [review]
v2

Some thoughts and questions:

* in jsdate.cpp: Should use SetNaNDate, instead of SetNanDate?
* is the change in definition of the JS_FN macro safe?  Don't we have external client code that uses this macro?  For their sake, and for the sake of minimizing this patch, can you just leave the macro the same, and ignore the minargs argument?
* is it necessary to keep the space needed for minargs in the function union?  Are we simply preserving alignment by using it?  Can the struct be reordered more optimally?
* What happens to client code that relies on minargs working?  Is this an API breakage we're comfortable with making now?  Better for Moz2?
* It seems like a diff -w would help with looking at the XML stuff, would you mind providing one?
* Does the browser build/work with this patch?  Mochis?

I looked pretty closely at everything but the XML stuff, but I'd like to look at it again with the -w, and look more closely at the rest again.

Comment 11

[Igor Bukanov]

Attachment: v2 as diff -w

(In reply to comment #10)
> * in jsdate.cpp: Should use SetNaNDate, instead of SetNanDate?

For me SetNaNDate looked ugly. Perhaps something like SetDateWithNaN ?

> * is the change in definition of the JS_FN macro safe?  Don't we have external
> client code that uses this macro?

JS_FN is an experimental interface subject to change.

> For their sake, and for the sake of
> minimizing this patch, can you just leave the macro the same, and ignore the
> minargs argument?

I prefer to break explicitly a hypothetical client rather then silently change the semantic of the macro. On the other hand with the current patch it is not easy to see which functions required code changes due to non-zero minarg. If that helps, I can create an intermediate patch with the change.

> * is it necessary to keep the space needed for minargs in the function union? 
> Are we simply preserving alignment by using it?  Can the struct be reordered
> more optimally?

JSFunction had the spare field before the fast native changes, see http://mxr.mozilla.org/mozilla1.8/source/js/src/jsfun.h#48 . The patch simply resurrected the field. The field indeed is used to fix the layout which should be optimal both on 32- and 64-bit CPUs.

> * What happens to client code that relies on minargs working?  Is this an API
> breakage we're comfortable with making now?  Better for Moz2?

The bug was scheduled for 1.9.0 but due to lack of time it was not done for the release. As regarding the breakage that would be the price for using an experimental API.

> * It seems like a diff -w would help with looking at the XML stuff, would you
> mind providing one?

Here it is.

> * Does the browser build/work with this patch?  Mochis?

The browser builds and mochis passes.

Comment 12

[Brian Crowder]

Comment on attachment 332704 [details] [diff] [review]
v2

Okay, on a second pass this looks good.  -w did help with the XML changes, thanks.

The arity-difference between JS_FS and JS_FN makes tables look a little weird now, but maybe the called-out difference is actually good.

I'm not attached on the NaN thing, but I think "Nan" is less readable, personally, especially since we use NaN nearly everywhere else.

I know of at least a couple of embeddings that will get bitten by the JS_FN signature change (especially, if they've changed old JS_FS routines to JS_FN, thinking the new macro would otherwise be semantically the same), so we should document this well for a 1.8 release.

Comment 13

[Mike Shaver (:shaver emeritus)]

SetDateToNaN?

Comment 14

[Brian Crowder]

SetDateToNaN works for me.

Comment 15

[Igor Bukanov]

Attachment: v2b

This patch is just the v2 version with SetNanDate -> SetDateToNaN rename.

Comment 16

[Igor Bukanov]

(In reply to comment #12)
> I know of at least a couple of embeddings that will get bitten by the JS_FN
> signature change (especially, if they've changed old JS_FS routines to JS_FN,
> thinking the new macro would otherwise be semantically the same),

That is very bad thinking: a fast native is not a slow native! For one thing one has to set vp as otherwise the function will return its js presentation instead of void. Another thing is the rules for accessing vp[1]. 

> so we should
> document this well for a 1.8 release.

Yes, this should be done, see bug 449651.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 332792 [details] [diff] [review]
v2b

Shouldn't js_ReportMissingArg try to decompile the value generator even if it's not of function-object type? The function's atom string should be fallback, for sure, but the DVG seems worth attempting no matter what type of callable object is on stack.

/be

Comment 18

[Igor Bukanov]

(In reply to comment #17)
> (From update of attachment 332792 [details] [diff] [review])
> Shouldn't js_ReportMissingArg try to decompile the value generator even if it's
> not of function-object type? 

The interpreter cannot call the fast mathed without a function object in vp[0]. So, when vp[0] is not a function in js_ReportMissingArg, then either some other code called the native method directly, or its implementation has already overwritten vp[0] before calling js_ReportMissingArg. In both cases it is pointless to use the decompiler AFAICS.

Comment 19

[Igor Bukanov]

Attachment: v3

The patch required a manual merge in js.cpp due to landing of bug 447713.

Comment 20

[Igor Bukanov]

landed - http://hg.mozilla.org/index.cgi/mozilla-central/rev/9e185457c656

Comment 21

[Igor Bukanov]

The patch is in 1.9.1 as it was landed before the 1.9.1 fork.

Comment 22

[ash_mozilla]

This patch has not landed in CVS and will be needed for JS1.8RC2. RC1 had the minargs flag in, but JS 1.7 did not, so this change should be safe to make.

Comment 23

[ash_mozilla]

Attachment: v3 against CVS

The attached v3 patch applies largely cleanly once all the extensions have been changed from .cpp back to .c, except these 3 files: js.c jsinterp.c jsobj.c jsxml.c

Comment 24

[ash_mozilla]

I can't count. thats 4 files.

Comment 25

[Jeff Walden [:Waldo]]

Why do the patches here not include the tests that were in my original WIP patch?  Or, failing that, why do the patches here not include tests that cover basically the same semantic ground?

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> Why do the patches here not include the tests that were in my original WIP
> patch?  

Sorry about missing that part. I guess I can land them in a separated bug. Is it OK?

Comment 27

[Jeff Walden [:Waldo]]

That would be fine, but the tests are only as complete as that patch was; they need to be updated to include tests for the other fastnative methods (preferably all, even if they previously had minargs == 0, but if my original tests only picked up the ones that were minargs != 0 I'm not going to complain, or at least not nearly as loudly :-) ).