Rockbox

Tasklist

FS#12186 - GSoC/Buflib: Put extended buflib into core

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Sunday, 10 July 2011, 00:01 GMT
Last edited by Thomas Martitz (kugel.) - Tuesday, 18 October 2011, 13:15 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.8.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch adds buflib (and the core_ wrappers) to the core and replaces almost all buffer_alloc() and buffer_get_buffer() [see  FS#12159 ]. The remaining calls are in target-specific code which I cannot verify so I'm post-poning the replacement there.

This patch depends on  FS#12159 .
This task depends upon

Closed by  Thomas Martitz (kugel.)
Tuesday, 18 October 2011, 13:15 GMT
Reason for closing:  Accepted
Additional comments about closing:  r30380
Comment by Thomas Martitz (kugel.) - Sunday, 10 July 2011, 00:13 GMT
Oh, I forgot to mention. For most places I didn't do the simple s/buffer_alloc()/buflib_alloc()/ replacement, but made the replacement in a way which should already work with compaction enabled. I.e. I implemented a (hopefully) functional move callback in some places.
Comment by Frank Gevaerts (fg) - Friday, 29 July 2011, 21:34 GMT
Some comments:

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/
Comment by Thomas Martitz (kugel.) - Saturday, 30 July 2011, 14:09 GMT
I didn't implement the discussed locking yet, because compaction isn't yet enabled (and as you said, I haven't looked closely at any of the proposals).

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.
Comment by Thomas Martitz (kugel.) - Wednesday, 03 August 2011, 21:02 GMT
This is an updated patch. It replaces the last occurences (although I still cannot really verify that, it looked pretty safe to me). It also removes some of the preliminary compaction handling to aid review. Plus, it implements the above discussed CANNOT_MOVE handling.

This is the first version I consider committable, so I kindly ask for test/review.
Comment by Thomas Martitz (kugel.) - Thursday, 04 August 2011, 11:22 GMT
Refined the patch a bit further, fixing a few bugs. This time I really added BUFLIB_CB_CANNOT_MOVE :)
Comment by Thomas Martitz (kugel.) - Tuesday, 09 August 2011, 18:49 GMT
I updated the patch to include some bug fixes.

I also add a second patch which enables compaction. Refer to the embedded commit messages for more information.
Comment by Thomas Martitz (kugel.) - Monday, 15 August 2011, 16:00 GMT
Hopefully the final iteration of the patches. They seem committable from my ongoing testing.

I resyncd them to SVN and fixed a few bugs in the compaction enablement patch. I also adapted timestretch to be runtime toggable.
Comment by Thomas Martitz (kugel.) - Sunday, 21 August 2011, 15:40 GMT
sync
Comment by sideral (sideral) - Monday, 22 August 2011, 17:35 GMT
(I still think that movable allocations introduce complexity and a maintenance burden that should be avoided, but you already knew that. ;-)

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.
Comment by Thomas Martitz (kugel.) - Tuesday, 30 August 2011, 12:51 GMT
I commit this without lots of changes (I fixed the typo, though).

> * 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.

Loading...