split memory manager into "standard" malloc/free and gc #1234
Conversation
…move mem functions to mem.c/h
|
Some comments:
Quick question:
Once the code has been ready to review and is almost ready for a pull, I will do the final work of separating the structures and cleaning up the |
|
Well, now this is at least something. But immediate comment is that this is pretty obtrusive, so may take quite a while to review. Some more comments:
I'd say that this splits memory subsystem (gc.c) into memory manager and garbage collector running on top of it.
The causation is opposite here - memory manager should provide them, because gc requires them. |
It should be clear that gc_init is not gc init, but memory manager init, and thus it is specific to a particular memory manager. |
|
More comments: On critical path to acceptance of this would be proof that this doesn't hurt performance of existing memmgr/gc implementation. Thus, having mem_sizeof() and friends as normal functions isn't good idea. Ideal performance form for them would be macros, as that guarantees inlinability in compiler-independent way. But that would be too cumbersome for human. Thus, they must be inline functions with a proof of inline (-Winline, assembly inspection). |
|
On the other hand, you should prove that gc/memmgr interface is powerful enough to implement gc on top of malloc with minimal memory overhead, which is set as 2 words. (Please forget about tinymem, there's only existing memory manager and malloc authoritative usecases now). |
|
mem.c is thus not mem.c, but something like mem_bitmapped_pool.c . The other implementation is mem_malloc.c . Maybe "memmngr_" prefix even would be more descriptive. |
|
There're lot of issues with codestyle. |
I think it is less intrusive if you just think of it this way: I moved all memory allocation/freeing to I changed very little about how anything worked (besides adding a few features like
Haha, well yes, I suppose that is true.
Two words (32 bit uints I assume?) should be doable. It would be through a different memmgr function built on top of malloc/free.
agreed
I'm sure I can fix them easily enough. I'll take a look at the style guide and see if I can spot them tomorrow. |
|
Also, it looks like it is failing compilation/tests on some of the embedded platforms. Any idea why that would be? |
Now prove that, if it's all green and red. |
You wrote the code and you're asking that? |
Things moved files, which is why they were "deleted" from one file and "added" to the other. Git doesn't pick up these kinds of diffs. This is a pretty major update, the whole point is that we completely remove a whole section of code from gc. |
It's not pool, it's not table. It's memory manager. But as memory manager manages memory, perhaps it's abbreviatable to just memory, i.e. "mem". |
Never compiled anything on a STM32 or most of these other platforms, especially not something as large as the micropython library. I'm wondering if they don't define their own object files in their makefile? This is the only thing I can think of, as everything should have remained cross platform. (except that I added a file) |
|
You do look at the actual failure(s), instead of guessing, right? https://travis-ci.org/micropython/micropython/builds/61521248#L1130 |
|
ah, there they are. Nevermind, I should be able to figure those out... (I've not used Travis before either) |
…other than mp_state and doesn't rely on gc ever
…d mem_free instead of custom solution (and standard free deals with this)
|
I've almost separated the data structures now. Still some work to be done. |
|
The major issue with separating the data structures is going to be how to initialize the memory pool. @pfalcon has suggested that How do you suggest we enable/disable certain memory managers? Very soon I will have the std malloc/free memory manager that can plug into |
|
I would like to continue work on this if there is still interest. What needs to be done to merge this? Because the memory manager is split, there has to be some solution for separating how memory is allocated for memory manager vs. garbage collector -- primarily the finalizer table (as they are now separate). Either that or we have to require all memory managers we use to include a finalizer table, which would be acceptable to me at this point. Thoughts? Based on @pfalcon's comments in #1279 there are other issues (because of splitting the files) but I am unsure of how else this should have been done and what the solution is now (complete rework that will end up in split files anyway?). Please comment. |
|
I am willing to rework this one commit at a time all in the same file if that is what is necessary to get this pulled. It will actually be fairly trivial. I would split it into the following PRs:
Each of these would be separate pull requests. Would this be acceptable? |
There's interest, thanks. |
|
So, I tried to do quick comments soon after patchset was posted, and hoped that other participants would review later, but that didn't happen. @dpgeorge should really look into this and tell how it should be, and other suggestion are "forward-looking statements". But otherwise, yes, rule of thumb is that changes should be:
For big changes, the above is rather hard to achieve, and well, that's art of programming and contribution of big enough patches to big enough projects. So yes, now that you have this draft patchset which shows where you want to arrive, I'd suggest leave it as is for reference, and submit localized changes PR by PR, each change should be small, and leave everything in a working state. |
|
Or I can tell how I'd do it - I'd make minimal set of changes to make uPy run with either existing memory allocator or with malloc/free. That would be the aim, and any change should be subordinare to it. If there's possibility to achieve the aim without splitting or renaming something, that's the way to go (while obviously doing everything in a generic manner). Splits and renames can be left for later cosmetic phase. |
|
awesome. #1320 is the beginning of my efforts. Is this about the right way to do things? |
Simple question - simple answer: no, it is not. That patch is codedrop of 420 lines, every second having codestyle problems. Start with a (useful!) patch of 10 lines, get it right, that's my suggestion. Other reviewers may have different opinions. |
|
10 changed lines of code is not going to split the memory manager. Decoupling I could of course do this completely piecewise, adding in functions that are already written only as they are "needed." Is this what you want or are you asking me to start from complete scratch? |
|
Ok, so if the above "how I'd do it" hints are not enough, let me be more specific:
Again, all the above is my personal opinion, and you can wait for other suggestions. |
|
I see what you mean. This is incredibly frustrating, as all the functionality was basically done and this is a significant amount of extra busy work to get done. I'm slightly worried that I'm going to do 8 hrs+ of work and it is going to be rejected again :/ I'll try to do it the way you suggest, by doing small commits and then getting your feedback. |
|
I find myself regularly in a position that my code is not merged, and I know how frustrating it is. Here, where I'm on the other side, being a maintainer, it's not less frustrating to see people doing a great work in not so right way, so their work is not directly mergeable without a lot of clean up on maintainers' side and leaps of faith. That's why I did that rant in #1279 try to send clear message that if people implement something to be merged, then they should care that it's easily mergeable, otherwise queues are long in every open-source project (damn, it's not that different in commercial projects, just less visible to outside). Steps to produce mergeable changes are not rocket science and are obvious as soon as you start to think about them - they are essentially to capture each separate and standalone step you do, and then do the steps in a separate and standalone way, without intuitive leaps back and forth.
You're not going to do 8hrs on that. You will spend 10-15 min to prepare patch for single function, if it passes review, you will go on with few next. At no time you would be spending 8hrs on it (well, maybe, after both you and maintainers are sure that code you'd produce is mergeable).
Thanks. 1 commit, and getting full feedback. Then reusing that experience for next commits (few again). |
|
@pfalcon I ended up doing the gc_sweep first, as it is a pretty simple use case of decoupling. Essentially two things have to be decoupled from the overall memory manager:
It ended up being a few more lines than 10, but I think it should meet all your qualifications. I tried to not add functionality that was already implemented like you said and keep everything as a macro. There are also comments on how things might be able to be sped up a bit, but I will leave it to you to determine how important that is. Thank you for taking the time to review my code and offer these tips. I would really like to be a core contributor to this project, and it is obvious that I need to learn the proper way to structure pull requests to do that. EditThe PR is #1321 |
Completes HALF of #1168
This pull request accomplishes the following:
next,sizeof,validandset_mark/clear_mark/get_markinterface. This is a valid requirement, as most micro memory managers I have seen have them (or it would be easy to add them) AND it saves significant space and time.Still to do (should be pretty straightforward)
gc.h