py/objtype: Define all special methods if requested. #3381
Conversation
|
Final step for #3291. Perhaps, we should have MICROPY_PY_INPLACE_SPECIAL_METHODS for consistency. I'm also open to suggestions that unix port should not now define MICROPY_PY_ALL_SPECIAL_METHODS (and what it should do instead). (Code size stats to follow.) |
|
Master with this patch (unix x86_64): Without patch, with special method mapping tables reverted to be of So, new MICROPY_PY_ALL_SPECIAL_METHODS gives +72 bytes overhead comparing to old non-optimized MICROPY_PY_ALL_SPECIAL_METHODS. IMHO, quite acceptable. And I guess, we should define MICROPY_PY_INPLACE_SPECIAL_METHODS, and have them off by default, and instead enable reverse ops for Unix port, as they bring more functionality than inplace ones. |
|
For more insight into code size changes: the changes which converted qstr to byte in the tables and sorted the special qstrs to the top reduced code size by: The changes in this PR introduce an increase of: So the NET change this PR brings compared to before any optimisations were made on the tables (which is the above numbers combined) is: Note that I'm using arm-none-eabi-gcc version 7.2.0, and gcc version 7.2.0 on Arch Linux which has PIC/PIE enabled by default, so the x64 binaries are much larger. |
|
An alternative is to generate the inplace and reverse qstrs on the fly, only when needed (by inserting i/r in the normal op names). But that may lead to heap allocation during an operation which is no good. |
If MICROPY_PY_ALL_SPECIAL_METHODS is defined, actually define all special methods (still subject to gating by e.g. MICROPY_PY_REVERSE_SPECIAL_METHODS). This adds quite a number of qstr's, so should be used sparingly.
This allows to configure support for inplace special methods separately, similar to "normal" and reverse special methods. This is useful, because inplace methods are "the most optional" ones, for example, if inplace methods aren't defined, the operation will be executed using normal methods instead. As a caveat, __iadd__ and __isub__ are implemented even if MICROPY_PY_ALL_INPLACE_SPECIAL_METHODS isn't defined. This is similar to the state of affairs before binary operations refactor, and allows to run existing tests even if MICROPY_PY_ALL_INPLACE_SPECIAL_METHODS isn't defined.
With inplace methods now disabled by default, it makes sense to enable reverse methods, as they allow for more useful features, e.g. allow for datetime module to implement both 2 * HOUR and HOUR * 2 (where HOUR is e.g. timedelta object).
pfalcon
force-pushed
the
spec-methods-all
branch
from
67f2b76
to
6a4149a
Compare
|
Ok, added MICROPY_PY_ALL_INPLACE_SPECIAL_METHODS setting, disabled by default (see commit message for caveats). Then, MICROPY_PY_REVERSE_SPECIAL_METHODS gets enabled for unix, which allows to use Going to merge this on weekend. |
|
It's codesize check what failed this time. (To clarify, with th latest change in #3381 (comment), unix port should stay sabout the same (inplace ops off, reverse on instead), while the rest of port should have size decreased beyond @dpgeorge's figures in #3381 (comment) , because those include inplace ops, which are now off by default (reverse had been off already)). |
|
Merged. |
Because |
The intention was to make options selecting special methods orthogonal. Apparently, before those were under ALL_SPECIAL_METHODS, but that makes less sense now that we have INPLACE_METHODS and REVERSE_METHODS, but keep having tests which assume We can put them back as they were, or can modify tests to use only |
Well, having 2**3 = 8 different options seems overkill in this situation. Likely there're only 3 configurations that are useful: minimal (all off), maximal (all on), some sensible middle ground which has the main useful ones enabled. I suggest to either 1) make it like configuring error messages and provide 3 options as described above; or 2) make the options a bit less confusing by renaming MICROPY_PY_ALL_INPLACE_SPECIAL_METHODS to MICROPY_PY_INPLACE_SPECIAL_METHODS and make it behave like the REVERSE option. Also I think #if MICROPY_PY_INPLACE_SPECIAL_METHODS
[MP_BINARY_OP_INPLACE_ADD] = MP_QSTR___iadd__,
[MP_BINARY_OP_INPLACE_SUBTRACT] = MP_QSTR___isub__,
[MP_BINARY_OP_INPLACE_MULTIPLY] = MP_QSTR___imul__,
#if MICROPY_PY_ALL_SPECIAL_METHODS
[MP_BINARY_OP_INPLACE_FLOOR_DIVIDE] = MP_QSTR___ifloordiv__,
[MP_BINARY_OP_INPLACE_TRUE_DIVIDE] = MP_QSTR___itruediv__,
[MP_BINARY_OP_INPLACE_MODULO] = MP_QSTR___imod__,
[MP_BINARY_OP_INPLACE_POWER] = MP_QSTR___ipow__,
[MP_BINARY_OP_INPLACE_OR] = MP_QSTR___ior__,
[MP_BINARY_OP_INPLACE_XOR] = MP_QSTR___ixor__,
[MP_BINARY_OP_INPLACE_AND] = MP_QSTR___iand__,
[MP_BINARY_OP_INPLACE_LSHIFT] = MP_QSTR___ilshift__,
[MP_BINARY_OP_INPLACE_RSHIFT] = MP_QSTR___irshift__,
#endif
#endif
[MP_BINARY_OP_ADD] = MP_QSTR___add__,
[MP_BINARY_OP_SUBTRACT] = MP_QSTR___sub__,
[MP_BINARY_OP_MULTIPLY] = MP_QSTR___mul__,
#if MICROPY_PY_ALL_SPECIAL_METHODS
[MP_BINARY_OP_FLOOR_DIVIDE] = MP_QSTR___floordiv__,
[MP_BINARY_OP_TRUE_DIVIDE] = MP_QSTR___truediv__,
[MP_BINARY_OP_MODULO] = MP_QSTR___mod__,
[MP_BINARY_OP_DIVMOD] = MP_QSTR___divmod__,
[MP_BINARY_OP_POWER] = MP_QSTR___pow__,
[MP_BINARY_OP_OR] = MP_QSTR___or__,
[MP_BINARY_OP_XOR] = MP_QSTR___xor__,
[MP_BINARY_OP_AND] = MP_QSTR___and__,
[MP_BINARY_OP_LSHIFT] = MP_QSTR___lshift__,
[MP_BINARY_OP_RSHIFT] = MP_QSTR___rshift__,
#endif
#if MICROPY_PY_REVERSE_SPECIAL_METHODS
[MP_BINARY_OP_REVERSE_ADD] = MP_QSTR___radd__,
[MP_BINARY_OP_REVERSE_SUBTRACT] = MP_QSTR___rsub__,
[MP_BINARY_OP_REVERSE_MULTIPLY] = MP_QSTR___rmul__,
#if MICROPY_PY_ALL_SPECIAL_METHODS
[MP_BINARY_OP_REVERSE_FLOOR_DIVIDE] = MP_QSTR___rfloordiv__,
[MP_BINARY_OP_REVERSE_TRUE_DIVIDE] = MP_QSTR___rtruediv__,
[MP_BINARY_OP_REVERSE_MODULO] = MP_QSTR___rmod__,
[MP_BINARY_OP_REVERSE_POWER] = MP_QSTR___rpow__,
[MP_BINARY_OP_REVERSE_OR] = MP_QSTR___ror__,
[MP_BINARY_OP_REVERSE_XOR] = MP_QSTR___rxor__,
[MP_BINARY_OP_REVERSE_AND] = MP_QSTR___rand__,
[MP_BINARY_OP_REVERSE_LSHIFT] = MP_QSTR___rlshift__,
[MP_BINARY_OP_REVERSE_RSHIFT] = MP_QSTR___rrshift__,
#endif
#endif |
I follow a different line: it's better to let users decide what they want, and it's maintainers' responsibility to make sure that's possible and generic, deconfused as much as reasonably possible. (But it's also maintainers' responsibility to set the default applying to most users, and as there's no single way to satisfy everyone, it always will be a compromise - and maintainers' are at better position to do that (than e.g. complaining users)). So, having 3 options, controlling 3 different groups of operators, seems pretty reasonable approach, and it even already gives its benefits - I was ready to enable all operators for unix port at all, but turned out, for the immediate milestone at hand (datatime module), only normal and reverse operators are required. Regarding the rest, all different options options were of course considered, and some even described above, e.g. why it was named MICROPY_PY_ALL_INPLACE_SPECIAL_METHODS (to not break existing tests). |
|
Ok, action items here:
|
|
Since there was no progress here, and spending effort to make small changes here seems counter productive, I'll close this. In 0a36a80 I updated a comment related to this PR. |
If MICROPY_PY_ALL_SPECIAL_METHODS is defined, actually define all special
methods (still subject to gating by e.g. MICROPY_PY_REVERSE_SPECIAL_METHODS).
This adds quite a number of qstr's, so should be used sparingly.