Additional function arg unpacking (PEP 448) #5807
Conversation
84a6ff9 to
129849f
Compare
|
might want to fix the typo in the title ;-) |
|
If that is my worst mistake, we are in good shape 😆 |
|
Thanks for working on this. It's a tough problem, especially to make it efficient (in RAM usage, execution time, code size). Properly reviewing this would take some time. For the record, the code size change is: |
…coded-samples audiomp3: add decoded_samples property
|
Can you please rebase this on latest master? |
b50221d to
d6aa8de
Compare
|
Rebased. |
Added a commit to address this todo item. |
IMO it makes sense to squash this commit into the first one, so that the
Do you mean it might segfault in an edge case? Or that it simply makes a bad guess at how much memory to preallocate? As far as I can tell it all looks OK. In fact this has exposed a bug in current master, the following will segfault: With this PR, that bug is fixed. |
|
I think the general approach here is very clever. When For simple cases: Also in the case of using I think this can be optimised. Eg if npos==0 then don't store the bitmap. And if npos==0 and nkw==0 then implicitly add 1 to nkw. |
Agreed.
The latter.
I will have a look. |
|
I have updated this with the suggested changes. With regard to the suggested optimizations, I did it a bit differently. " if npos==0 then don't store the bitmap." was simple enough but "And if npos==0 and nkw==0 then implicitly add 1 to nkw." got messy since it requires checking for this in many places. So, I figured that if we were going to add all of checks everywhere anyway, we could do it slightly differently and optimize (at least in terms of fewer bytes emitted) more cases. See the commit message for the full explanation. Given that arg unpacking is probably not frequently used, I'm not sure the trade off in compiler size/complexity is worth the savings in emitted byte code size. |
You might be right, it does look complicated. I will review it. |
|
FWIW I think this is a very useful feature - subject to code size, of course :) For example in GUI code, widgets may have multiple constructor args to define colors, shapes, fonts, sizes, callbacks etc. A |
tests/basics/fun_kwargs.py
Outdated
| return x | ||
|
|
||
|
|
||
| f4(*print_ret(["a", "b"]), kw_arg=print_ret(None)) |
There was a problem hiding this comment.
this test should go in tests/baics/fun_kwvarargs.py
|
Code size change for supporting multiple Then the additional code size change on top of that for the encoding optimisation is: I'd say that optimisation is not worth it. It's relatively rare to use So please remove that encoding optimisation commit from this PR. That should also fix the CI issues with native emitter. |
py/vm.c
Outdated
| @@ -927,8 +927,8 @@ unwind_jump:; | |||
| // unum & 0xff == n_positional | |||
| // (unum >> 8) & 0xff == n_keyword | |||
| // We have following stack layout here: | |||
| // fun arg0 arg1 ... kw0 val0 kw1 val1 ... seq dict <- TOS | |||
| sp -= (unum & 0xff) + ((unum >> 7) & 0x1fe) + 2; | |||
| // fun arg0 arg1 ... kw0 val0 kw1 val1 ... bitmap <- TOS | |||
There was a problem hiding this comment.
In this commit the bitmap does not exist so it should be seq instead. I.e., this commit should just delete the dict entry. Then in the next commit, seq is renamed to bitmap.
|
Performance change (without the encoding optimisation commit), on PYBv1.0: |
|
I have made the requested changes, dropped the extra commits and rebased. |
Codecov Report
@@ Coverage Diff @@
## master #5807 +/- ##
==========================================
- Coverage 98.24% 98.24% -0.01%
==========================================
Files 154 154
Lines 20273 20286 +13
==========================================
+ Hits 19918 19929 +11
- Misses 355 357 +2
Continue to review full report at Codecov.
|
|
The changes in code coverage show some potential issues:
|
|
I have added a commit to address the overallocation issue. But it doesn't seem to affect the code coverage, so there is probably something else I am missing. |
|
I've added a few more commits that should take care of the dropped coverage issues. So the only question that remains is should we add a test that duplicates the cpydiff to get the last bit of coverage or should we just include all cpydiff in the coverage run or don't worry about coverage for that branch? |
This is a partial implementation of PEP 448 to allow multiple ** unpackings when calling a function or method. The compiler is modified to encode the argument as a None: obj key-value pair (similar to how regular keyword arguments are encoded as str: obj pairs). The extra object that was pushed on the stack to hold a single ** unpacking object is no longer used and is removed. The runtime is modified to decode this new format. Signed-off-by: David Lechner <david@pybricks.com>
dcd0bdb to
12755dc
Compare
Yes I think we should add another test for this in the main test suite (this is to check when there are too many positional args, right?). Because cpydiff tests are intended to be changed/removed if/when the feature becomes compatible with CPython. Thus cpydiff tests cannot be relied upon for coverage. |
This is a partial implementation of PEP 448 to allow unpacking multiple star args in a function or method call. This is implemented by changing the emitted bytecodes so that both positional args and star args are stored as positional args. A bitmap is added to indicate if an argument at a given position is a positional argument or a star arg. In the bytecodes, this new bitmap takes the place of the old star arg. It is stored as a small int, so this means only the first N arguments can be star args where N is the number of bits in a small int. The runtime is modified to interpret this new bytecode format while still trying to perform as few memory reallocations as possible. Signed-off-by: David Lechner <david@pybricks.com>
This fixes overallocating an extra mp_obj_t when the length of *args and **args is known. Previously we were allocating 1 mp_obj_t for each n_args and n_kw plus the length of each *arg and **arg (if they are known). Since n_args includes *args and n_kw includes **args, this was allocating an extra mp_obj_t in addition to the length of these args when unpacked. To fix this, we just subtract 1 from the length to account for the 1 already implicitly allocated by n_args and n_kw. Signed-off-by: David Lechner <david@pybricks.com>
To reach this check, n_kw has to be >= 1 and therefore args2_alloc has to be >= 2. Therefore new_alloc will always be >= 4. So this check will never be true and can be removed. Signed-off-by: David Lechner <david@pybricks.com>
This fixes code coverage for the case where a *arg without __len__ is unpacked and uses exactly the amount of memory that was allocated for kw args. This triggers the code branch where the memory for the kw args gets reallocated since it was used already by the *arg unpacking. Signed-off-by: David Lechner <david@pybricks.com>
This replaces instances of uint with size_t and int with ssize_t in the mp_call_prepare_args_n_kw_var() function since all of the variables are used as array offsets. Also sort headers while we are touching this. Signed-off-by: David Lechner <david@pybricks.com>
|
I have a few extra coverage tests to add in a separate PR. |
This is an attempt to implement PEP 448 for function arguments. It allows multiple *args and **args to be unpacked. (Popular request in #1329).
I'm curious to see how it affects code size and performance.
There are still some todo items: