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

Use mpy map and heap where possible, use root pointer #16

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

swuerl
Copy link

@swuerl swuerl commented Nov 13, 2024

At this point this is more an RFC than a ready PR, as it changes a lot about how micropython-wrap works at the core. Please let me know if you're generally open for merging something like this, maybe behind a `#ifdef' or similar if you want to maintain the old behaviour as well.

My motivation is: We're using micropython-wrap since a while now on a freestanding C++ implementation on a microcontroller. We have no STL (vector, map) available. Also, apart from the micropython garbage collected heap no global heap for C/C++ allocations.

Therefore we replaced the map uses by maps stored on the micropython heap. This of course requires consideration for the garbage collector, as to not accidentally clean up the micropython-wrap generated bindings.

As a nice side effect, we need to neither store the <class>_globals nor the _StaticPyObjectStore in a user-available location.

This replaces the use of std::map by micropython heap based maps. As a result, we need a root pointer for the root map such that the mpy garbage collector does not collect the micropython-wrap objects.

This root pointer needs to be declared once outside of micropython-wrap such that it is generated by the micropython generators.

All objects referenced by the map should also be on the micropython-heap. In this way, the garbage collector can properly track lifetimes.

This is the first step into allowing us to use micropython-wrap on mcus, and also allows us to reset the micropython-wrap state by resetting the global state map.

This replaces the use of std::map by micropython heap based maps.
As a result, we need a root pointer for the root map such that the
mpy garbage collector does not collect the micropython-wrap objects.

This root pointer needs to be declared once outside of micropython-wrap
such that it is generated by the micropython generators.

All objects referenced by the map should also be on the
micropython-heap. In this way, the garbage collector can properly
track lifetimes.

This is the first step into allowing us to use micropython-wrap on mcus,
and also allows us to reset the micropython-wrap state by resetting the
global state map.
@swuerl swuerl marked this pull request as draft November 13, 2024 14:40
@stinos
Copy link
Owner

stinos commented Nov 14, 2024

Please let me know if you're generally open for merging something like this,

Thanks for the interest, and yes, the overall idea is definitely sound and this would be a welcome addition. Very early on I considered making the whole allocation policy pluggable, exactly for the reasons you're doing it now and others, but IIRC back then there was no easy build support like MP_REGISTER_ROOT_POINTER so it went the other way.

maybe behind a `#ifdef' or similar if you want to maintain the old behaviour as well.

Preferably, because that would make it easier to switch implementations for testing than having to change branches.

As for the actual shape of this:

  • code-wise: your MPyMapView already goes a long way in keeping things readable and minmizes changes needed, but I wonder if some of the other allocation-related changes could also be abstracted away somehow? To reduce the number of ifdefs needed basically, and to (possibly) allow future changes as well. Like using a separate micropython heap (because a lot of the objects are allocated once and should never be GC'd so might make sense to put them somewhere where the GC isn't contantly looking). Note this is just an idea, I didn't check in detail if possible.
  • commit-wise: would be great if you can split this the usual way i.e. granular commits for each distinct change, that's going to be easier to look at. Just to name something: instead of replacing &type with type_ptr in a bunch of places make that a separate commit which introduces a static mp_obj_type* TypePtr() or something like that to use instead of &type such that a next commit changing from holding an mp_obj_full_type_t * instead of mp_obj_full_type_t doesn't have to touch all locations where the type is used. Introduction of ClassHash is another commit. And so on. Some of these features might also need explicit tests, not sure.

This is the first step into allowing us to use micropython-wrap on mcus, and also allows us to reset the micropython-wrap state by resetting the global state map.

Can you lay out other steps? Removing exception handling perhaps? Making some type conversions (strings etc) optional?

@swuerl
Copy link
Author

swuerl commented Nov 29, 2024

I wonder if some of the other allocation-related changes could also be abstracted away somehow?

Yes, I think so. For one of the future changes I'd like to introduce an STL compatible custom allocator. I'll describe below why that's needed. This could be used/aliased to either be the default allocator (as implicitly used right now), or the custom allocator that allocates on the mpy heap.

Like using a separate micropython heap

I think we'd need either an entirely seperate heap implementation here (which I would like to avoid) or this is probably not feasible. The micropython heap is quite global.
At least on my end, I also use this as a feature: We're fully resetting the micropython engine at runtime which includes clearing the entire micropython heap. The way I've implemented that in our fork is that micropython-wrap will also fully automatically reload the modules, including all classwrappers.

would be great if you can split this the usual way

Yes, will do that. Wanted to post this PR to kind of show the direction this is going.

Can you lay out other steps?

Next steps are:

  • Adding custom allocators (reference to above mention of custom allocators) to all shared_ptr constructors (this can again also be the default constructor), this allows to use shared pointers with the micropython-heap
  • Replace uses of std::string (mostly used in exception handling) and std::vector (in argument map) by heapless alternatives
  • Remove use of typeid().name()
  • (Optional): Add C++23 std::move_only_function wrapper for FromPyObj wrappers to allow heap-less capturing of the pinned py object

Note: I'm not able to transform this into a full -ffreestanding implementation. I think this is a lot more complex. My target is an environment with STL headers, but without STL lib (as e.g. the official arm-none-eabi GCC toolchain provides). But the patches would bring a freestanding implementation a lot closer, potentially making that possible with future patches.

@swuerl
Copy link
Author

swuerl commented Nov 29, 2024

I've created a first PR here: #18

I'd continue to slowly pull out small steps as PRs to make the allocator and map implementations pluggable.

@swuerl
Copy link
Author

swuerl commented Nov 29, 2024

Another one for ClassHash: #19

@swuerl
Copy link
Author

swuerl commented Nov 29, 2024

I also have another one introducing the root-pointer and MPyMapView in a very non-intrusive way: #20

@stinos
Copy link
Owner

stinos commented Dec 3, 2024

Next steps are:

Ok thanks for the explanation. All sounds pretty sensible. Practical issue for me is that the end of the year traditionally gets super busy so I don't know when I'll have time to review in detail.

@swuerl
Copy link
Author

swuerl commented Dec 3, 2024

Makes sense, no hurry. If thats ok for you, I'd continue with separating out small nice PRs (and hopefully making the CI work), and we can discuss that whenever you have the time.

My team and me are already using an internal fork of micropython-wrap that works with our target toolchain, and I'm just trying to contribute that back upstream, but from my side there is no urgency in this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants