Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

BF-31 #8

Merged
merged 59 commits into from
Apr 14, 2023
Merged

BF-31 #8

merged 59 commits into from
Apr 14, 2023

Conversation

zollqir
Copy link
Collaborator

@zollqir zollqir commented Mar 22, 2023

https://distributive.atlassian.net/jira/software/c/projects/BF2/issues/BF2-31

This MR implements coercion for python strings to javascript strings, and python functions to javascript functions.

zollqir and others added 30 commits March 14, 2023 11:52
Xmader added a commit that referenced this pull request Mar 28, 2023
Comment on lines 117 to 122
PyObject *const inspect = PyImport_Import(PyUnicode_DecodeFSDefault("inspect"));
PyObject *const getfullargspec = PyObject_GetAttrString(inspect, "getfullargspec");
PyObject *const getfullargspecArgs = PyTuple_New(1);
PyTuple_SetItem(getfullargspecArgs, 0, object);
PyObject *const argspec = PyObject_CallObject(getfullargspec, getfullargspecArgs);
PyObject *const args = PyObject_GetAttrString(argspec, "args");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a real use of argument count in JS?

JS functions should be all variadic in nature. (Not verified yet)

Copy link
Collaborator Author

@zollqir zollqir Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be used by the JIT compiler for optimization purposes, but apart from that I don't think so

Comment on lines 117 to 124
PyObject *const inspect = PyImport_Import(PyUnicode_DecodeFSDefault("inspect"));
PyObject *const getfullargspec = PyObject_GetAttrString(inspect, "getfullargspec");
PyObject *const getfullargspecArgs = PyTuple_New(1);
PyTuple_SetItem(getfullargspecArgs, 0, object);
PyObject *const argspec = PyObject_CallObject(getfullargspec, getfullargspecArgs);
PyObject *const args = PyObject_GetAttrString(argspec, "args");

JSFunction *jsFunc = js::NewFunctionWithReserved(cx, callPyFunc, PyList_Size(args), 0, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maximum argument count in SpiderMonkey is 65535 (haven't tested yet)
https://hg.mozilla.org/releases/mozilla-esr102/file/tip/js/src/vm/JSFunction.cpp#l1895

What's the limit in CPython? Would nargs overflow?

Copy link
Collaborator Author

@zollqir zollqir Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some research, looks like before python 3.7, maximum number of arguments is 255, after 3.7 maximum number of arguments is theoretically unbounded (though realistically its bounded by sys.maxsize, which is 2^64 - 1 on 64-bit architectures).

So I'm thinking we should error out if nargs > 65535. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PythonMonkey can actually handle such huge amount (> 65535) of arguments to a variadic function

import pythonmonkey as pm

js_fn = pm.eval("(...args) => { return args.length }")

def py_fn(*args):
  return len(args)

l = range(2**16+1)
print(py_fn(*l)) # 65537
print(js_fn(*l)) # 65537.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SpiderMonkey's limit is actually 500k arguments

https://hg.mozilla.org/releases/mozilla-esr102/file/tip/js/src/vm/Stack.h#l879

    if (argc > ARGS_LENGTH_MAX) {
      JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                                JSMSG_TOO_MANY_ARGUMENTS);
      return false;
    }

https://hg.mozilla.org/releases/mozilla-esr102/file/tip/js/src/vm/ArgumentsObject.h#l95

// Maximum supported value of arguments.length. This bounds the
// maximum number of arguments that can be supplied to a spread call
// or Function.prototype.apply.  This value also bounds the number of
// elements parsed in an array initializer.  NB: keep this in sync
// with the copy in builtin/SelfHostingDefines.h.
static const unsigned ARGS_LENGTH_MAX = 500 * 1000;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified in PythonMonkey

import pythonmonkey as pm
js_fn = pm.eval("(...args) => { return args.length }")
def py_fn(*args):
  return len(args)

l = range(500 * 1000)
print(py_fn(*l)) # 500000
print(js_fn(*l)) # 500000.0

l = range(500 * 1000 + 1)
print(py_fn(*l)) # 500001
print(js_fn(*l)) # pythonmonkey.SpiderMonkeyError: RangeError: too many arguments provided for a function call

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary to error it out because

  • no one would write a Python function with more than 500k formal arguments
  • SpiderMonkey would error it out when passing more than 500k arguments to a JS function
  • inspect.getfullargspec(py_fn).args only contains positional parameter names, returns a empty list for the py_fn function above

zollqir and others added 4 commits April 12, 2023 14:04
Co-authored-by: Tom Wenzheng Tang <xmader@distributive.network>
Co-authored-by: Tom Wenzheng Tang <xmader@distributive.network>
BF-42: UCS4-range strings do not work properly when passed as arguments to JS-defined functions
…-called `global` argument is actually the `this` value to a JS function
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants