Skip to content
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

py: support mutable attributes on sys module #7591

Merged
merged 4 commits into from Mar 10, 2022

Conversation

dpgeorge
Copy link
Member

This PR adds generic support for mutable module attributes on built in modules, by adding support for an optional hook function for module attribute lookup. If a module wants to support additional attribute load/store/delete (beyond what is in the constant, globals dict) then it should define at the end of its globals dict an entry with MP_QSTR_NULL key and value a pointer to a function of type mp_attr_fun_t. Then that function will handle any additional attributes (which can include stores).

To show how this works, this PR also adds mutable attributes to the sys module, where they are really needed:

  • sys.stdout is now writable
  • sys.tracebacklimit can be set to an integer to limit the stored traceback (set to 0 disables traceback information, to save RAM)
  • sys.ps1 and sys.ps2 can be modified to customise the REPL

This is WIP and RFC... there are many ways to make module attributes mutable and the way proposed here aims to minimise RAM usage, keep code size down to a minimum, and make it general for other modules to use.

@stinos
Copy link
Contributor

stinos commented Jul 26, 2021

it should define at the end of its globals dict an entry with MP_QSTR_NULL key

I assume that's because it is easy to implement/ fast to lookup, but it doesn't look very convenient when creating modules via mp_new_module and then populating its globals with mp_obj_dict_store: it requires 'special' code to add he entry (mp_map_lookup increments alloc by 4 when adding items so probably need to manually set map->table[map->alloc - 1] key and value? Perhaps there should be a macro to do that then?) and it's required that the entry is added last (not sure what would happen when trying to store more items afterwards).

But for the rest the idea is sound.

@dpgeorge
Copy link
Member Author

I assume that's because it is easy to implement/ fast to lookup

Yes. It could also go as the first entry, but then every normal dict lookup would have to search that entry unnecessarily.

but it doesn't look very convenient when creating modules via mp_new_module and then populating its globals with mp_obj_dict_store

I didn't think of this case. But is it really needed to support it? In the situation where mp_obj_new_module() is used, the dict is in RAM anyway and attributes can be added/changed without a problem.

@dpgeorge dpgeorge force-pushed the py-sys-mutable-attrs branch 4 times, most recently from 1d190a8 to 5d90183 Compare July 27, 2021 01:34
@stinos
Copy link
Contributor

stinos commented Jul 27, 2021

But is it really needed to support it?

Good point, probably not. load/store to a module is probably always going to implemented as such, i.e. just load and store, no custom hook function, so indeed in case of a RAM dict the default load/store should be ok.

@dpgeorge
Copy link
Member Author

Actually, considering a constant/fixed and linear (not hash table) dict is fully searched to find an attribute before failing and falling back to this delegation function, it could be possible to, as part of the initial search, remember the special MP_QSTRnull entry if found so it can be immediately used at the end.

@dpgeorge dpgeorge force-pushed the py-sys-mutable-attrs branch 5 times, most recently from e08a61a to efd489b Compare March 9, 2022 09:06
@dpgeorge dpgeorge changed the title py: support mutable attributes on sys module (WIP) py: support mutable attributes on sys module Mar 9, 2022
@dpgeorge
Copy link
Member Author

dpgeorge commented Mar 9, 2022

I removed the mutable sys.stdout change, because it wasn't fully correct in semantics compared to CPython. Can be implemented another time if needed.

As it stands, this PR adds sys.ps1 and sys.ps2 to builds with MICROPY_CONFIG_ROM_LEVEL_AT_LEAST_EXTRA_FEATURES and sys.tracebacklimit with MICROPY_CONFIG_ROM_LEVEL_AT_LEAST_EVERYTHING. Change in code size is:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% 
unix nanbox:    +0 +0.000% 
      stm32:  +256 +0.065% PYBV10[incl +8(bss)]
     cc3200:    +0 +0.000% 
    esp8266:    +0 +0.000% GENERIC
      esp32:  +308 +0.020% GENERIC[incl +48(data) +8(bss)]
        nrf:    +0 +0.000% pca10040
        rp2:  +232 +0.047% PICO[incl +8(bss)]
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Performance is unchanged (at least within measurement errors).

Tests have been added and docs updated. This is ready to merge.

This commit adds generic support for mutable module attributes on built in
modules, by adding support for an optional hook function for module
attribute lookup.  If a module wants to support additional attribute load/
store/delete (beyond what is in the constant, globals dict) then it should
add at the very end of its globals dict MP_MODULE_ATTR_DELEGATION_ENTRY().
This should point to a custom function which will handle any additional
attributes.

The mp_module_generic_attr() function is provided as a helper function for
additional attributes: it requires an array of qstrs (terminated in
MP_QSTRnull) and a corresponding array of objects (with a 1-1 mapping
between qstrs and objects).  If the qstr is found in the array then the
corresponding object is loaded/stored/deleted.

Signed-off-by: Damien George <damien@micropython.org>
To be enabled when needed by specific sys attributes.

Signed-off-by: Damien George <damien@micropython.org>
With behaviour as per CPython.

Signed-off-by: Damien George <damien@micropython.org>
This allows customising the REPL prompt strings.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge merged commit ac22931 into micropython:master Mar 10, 2022
@dpgeorge dpgeorge deleted the py-sys-mutable-attrs branch March 10, 2022 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants