-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
internal: more uniform usage of jlcall ABI again #31984
Conversation
JL_GC_POP(); | ||
return ret; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Yeah, this was a super ugly function.
Just to check: @nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
Curious, it doesn't seem that we have any benchmarks affected by the simplified splatting calling. But this seems to have broken something about parse_json and binary_tree. I'll have to look into those. |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
Looks good now. Did parse_json and binary_tree actually reveal something, or were they noise? |
yeah, I had accidentally disabled an optimization. it's all good now, so I'll plan to merge tomorrow |
A bit of history: Before the addition of function types, a call to a function would always use the API
(jl_value_t *f, jl_value_t *argv[], int nargv)
, regardless of whether it was a generic call or a closure or an invoke or interpreted. Over time since then, that become one of several calling conventions, each just a minor variation away:(jl_value_t *argv[], int nargv)
,(void *context, jl_value_t *argv[], int nargv)
,(jl_value_t *f, jl_value_t *argv[], int nargs)
,(jl_value_t **context, jl_value_t *f, jl_value_t *argv[], int nargv)
, and so on. That was fairly nice to getting stuff ported over, since it provided a simpler transition, but led to needing an increasing number of transformational methods to just shift arguments around (such asjl_apply_2va
, which gets removed here). And that was starting to feel tricky to me to manage the possible N-to-N conversions that may be needed between each of them, and to optimize all of those.This PR aims to cut back down again on the amount of variety in our primary (non internal/specsig) ABI by moving the varying 'context' to the end and usually keeping the first (function) argument separate. This gives the basic call signature form:
(jl_value_t *func, jl_value_t *argv[], int nargv, void *optional_context)
(where the last parameter can range over a few possible types depending on the target, or may beundef
and dropped when it is known that the callee will ignore the value). This lets us switch callee target type (e.g. for incremental compilation and linking) simply by adding / dropping / replacing / ignoring that trailing parameter, and not needing to potentially spill everything to the stack (e.g. letting us remove jl_apply_2va).This doesn't alter
jl_apply()
/jl_call
/ etc., so external consumers should be generally unaffected.