This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#12186 - GSoC/Buflib: Put extended buflib into core
Attached to Project:
Rockbox
Opened by Thomas Martitz (kugel.) - Sunday, 10 July 2011, 02:01 GMT+2
Last edited by Thomas Martitz (kugel.) - Tuesday, 18 October 2011, 15:15 GMT+2
Opened by Thomas Martitz (kugel.) - Sunday, 10 July 2011, 02:01 GMT+2
Last edited by Thomas Martitz (kugel.) - Tuesday, 18 October 2011, 15:15 GMT+2
|
DetailsThis patch adds buflib (and the core_ wrappers) to the core and replaces almost all buffer_alloc() and buffer_get_buffer() [see
This patch depends on 
buflib-core.patch
(69.9 KiB)
b/apps/debug_menu.c | 36 + b/apps/dsp.c | 28 - b/apps/filetypes.c | 53 +- b/apps/gui/skin_engine/skin_engine.c | 3 b/apps/main.c | 6 b/apps/playback.c | 37 + b/apps/playlist.c | 44 + b/apps/playlist.h | 6 b/apps/scrobbler.c | 15 b/apps/tagcache.c | 13 b/apps/tagtree.c | 12 b/apps/talk.c | 9 b/apps/tdspeed.c | 65 ++ b/apps/tree.c | 10 b/firmware/SOURCES | 2 b/firmware/buflib.c | 726 ++++++++++++++++++++++++++++++ b/firmware/common/dircache.c | 37 + b/firmware/core_alloc.c | 61 ++ b/firmware/include/buflib.h | 238 +++++++++ b/firmware/include/core_alloc.h | 34 + b/firmware/rolo.c | 8 b/firmware/target/hosted/sdl/button-sdl.c | 2 22 files changed, 1356 insertions(+), 89 deletions(-) |
This task depends upon
Closed by Thomas Martitz (kugel.)
Tuesday, 18 October 2011, 15:15 GMT+2
Reason for closing: Accepted
Additional comments about closing: r30380
Tuesday, 18 October 2011, 15:15 GMT+2
Reason for closing: Accepted
Additional comments about closing: r30380
Is the change to firmware/target/hosted/sdl/button-sdl.c meant to be in this patch?
We discussed this on irc, but I don't remember the exact outcome. I believe there should be a way to let memory users *temporarily* lock a handle (there are several cases where data is transferred to or from disk at boot or shutdown, e.g. font cache, database load,.... It would be a pity if those would have to be unmovable allocations due to only this very small part of their lifetime).
IIRC you suggested a function to set a new struct buflib_callbacks for a handle. Other possibilities could be to allow move_callback() to e.g. return BUFLIB_CB_CANNOT_MOVE, or to add buflib_lock() and buflib_unlock() functions.
I think all of those are equivalent in the end, but I suspect it might be a good idea to postpone a final decision here until you've actually looked at (at least) one of those cases in a bit of depth.
Other than that, I think the changes look sane to me, but I'm not an expert for most of apps/
I had BUFLIB_CB_CANNOT_MOVE in there in my api planning phase (it was actually BUFLIB_CB_DEFER_MOVE). I think this would perhaps be the best proposal because it doesn't introduce locking schemes into buflib.
It's functionally eqivalent to lock/unlock, just that the buflib user would have those functions to set a local variable which is then checked in the callback. It would also move allocation of the lock data structure (a bool, most probably) to the user and keep the metadata size in buflib smaller.
I'm reconsidering my previous favor for the function to set the callbacks, you had some valid points when arguing against.
This is the first version I consider committable, so I kindly ask for test/review.
I also add a second patch which enables compaction. Refer to the embedded commit messages for more information.
I resyncd them to SVN and fixed a few bugs in the compaction enablement patch. I also adapted timestretch to be runtime toggable.
I've had a look at the tagcache / tagtree / dircache changes, and they look mostly sane to me. Here are a few more specific comments:
Patch 0001:
* Typo: "scrobller"
* tagcache: Why did you increase the tc ramcache padding from 128 to 256 bytes?
* dircache: Is there a reason not to aggregate opendir_dnames inside opendirs[]
Patch 0002:
* tagcache: The macro expansion in tagcache_fill_tags looks expensive (in terms of binsize). Consider using a loop similar to r29937 /
FS#12136.* tagtree: struct tagentry: Rather than relying on compatible layout with struct entry, consider aggregating a common struct type and keeping only arrays of pointers to that type.
* To enable moving pointers, in some modules you introduce unions with char* pointers, whereas in others you rely on type casting to char* when needed. It would be good if you unified this. I'd prefer the latter approach.
> * tagcache: Why did you increase the tc ramcache padding from 128 to 256 bytes?
The padding isn't increased. I merged the 128 from ramcache_allocated and the 128 from the buffer_alloc() call to 256.
> * dircache: Is there a reason not to aggregate opendir_dnames inside opendirs[]
struct dirent_cached as a d_name pointer, not a char array, so it's not easy to aggregate. I prefer to keep the changes minimal.
> * tagcache: The macro expansion in tagcache_fill_tags looks expensive (in terms of binsize). Consider using a loop similar to r29937 /
FS#12136.Will have a look
> * tagtree: struct tagentry: Rather than relying on compatible layout with struct entry, consider aggregating a common struct type and keeping only arrays of pointers to that type.
I considered this, but it's hacky one way or the other...so I took the route of less changes to keep the diff smaller.
> * To enable moving pointers, in some modules you introduce unions with char* pointers, whereas in others you rely on type casting to char* when needed. It would be good if you unified this. I'd prefer the latter approach.
Casting doesn't work on the left-hand side of += so I used the unions. I don't care much between between cast and unions, but the unions seem a little cleaner to me, since they don't prevent compiler checks.