Skip to content

[RFC] Sys builtin module names V2 #4553

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

Closed
wants to merge 2 commits into from

Conversation

Jongy
Copy link
Contributor

@Jongy Jongy commented Feb 25, 2019

A second approach, as suggested by @dpgeorge in #4468 (#4468 (comment))

I don't think it's that bad. Only if you want the tuple does it use any RAM (and it can then be reclaimed). Also, it'd be possible to add a generic hook for any module to define its own special attributes via a callback that is part of the module's data structure. That would be less of a hack.

Currently tested on unix, didn't test sizes yet / on any other platform.

Happy for comments :)

@Jongy Jongy changed the title [RFC} Sys builtin module names V2 [RFC] Sys builtin module names V2 Feb 25, 2019
@Jongy Jongy force-pushed the sys-builtin-module-names-v2 branch from f9990e2 to f26023f Compare February 27, 2019 22:36
@Jongy
Copy link
Contributor Author

Jongy commented Feb 27, 2019

Tested on the ESP32. Works (duh) but doesn't handle soft resets very well - after reading the code and realizing how soft resets work, I came to a conclusion that randomly placing global, lazily initialized variables won't work so well...

Need to figure out how to do this nicer. Perhaps if there was a special data section that mp_init would automatically run memset(0) on, or something like that.

@Jongy
Copy link
Contributor Author

Jongy commented Feb 27, 2019

Need to figure out how to do this nicer. Perhaps if there was a special data section that mp_init would automatically run memset(0) on, or something like that.

Otherwise, the only solution I see is to manually add each item to MP_STATE_VM. And by the way, perhaps some, or even many, of the items in MP_STATE_VM could be moved to such a section, instead of being selectively placed in a global struct. I'd find it nicer.

@pfalcon
Copy link
Contributor

pfalcon commented Feb 27, 2019

I came to a conclusion that randomly placing global, lazily initialized variables won't work so well...

What made you give up on previous approach of compile-time tuple generation?

@Jongy
Copy link
Contributor Author

Jongy commented Feb 27, 2019

What made you give up on previous approach of compile-time tuple generation?

You mean #4468?

@pfalcon
Copy link
Contributor

pfalcon commented Feb 27, 2019

You mean #4468?

Yep.

@Jongy
Copy link
Contributor Author

Jongy commented Feb 27, 2019

Can't say I've given up on it. I still think it's a better approach. But I wanted to gather some more approval before continuing with it, and meanwhile I thought to offer this PR to show another softer, possible way, and see what's favoured more.

If you think the first solution is better and viable, I'll continue with it.

@pfalcon
Copy link
Contributor

pfalcon commented Feb 27, 2019

Yes, I personally think that having fully static, ROMable data is better. And this "lazy globals" stuff is too magic. Before going for things like that, we'd rather firsy solve the issue of letting override sys.stdout and friends, I probably already mentioned that.

@dpgeorge
Copy link
Member

Thanks for the attempt @Jongy . You can see that getting all the fine details correct is not easy. The cached list of builtins would need to be stored in the root pointer section (as you found out) making it more complex. And a downside of the approach here is that (with the feature enabled) all modules grow by 1 word, even though most won't use the lazy feature. That could be solved by putting the lazy function pointer in the modules dict (somehow), or making mp_obj_module_t flexible in size (eg there is mp_obj_module_extra_t which has this extra lazy pointer only for those modules that use it).

At this point I don't think the utility of having sys.builtin_module_names outweighs the complexity introduced here in the implementation. That could change if there were other compelling uses for the lazy attr (which could be extended to store, not just load).

The ROM approach is probably going to be even more complicated, but hard to say until there is a working proof of concept. In the end there may not be an elegant way to implement the feature.

@Jongy
Copy link
Contributor Author

Jongy commented Mar 31, 2019

The ROM approach is probably going to be even more complicated, but hard to say until there is a working proof of concept

I agree, basically :)

Since this approach really got too complicated, and since I already have a functioning PoC of the ROM approach, I'll close this PR. Back to #4468.

@Jongy Jongy closed this Mar 31, 2019
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Apr 8, 2021
decompress: Fix decompression when length takes 7 bits
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.

3 participants