Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Operating System/Drivers
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.8.1
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Thomas Martitz - 2011-07-10
Last edited by Thomas Martitz - 2011-10-18

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

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 .

Closed by  Thomas Martitz
2011-10-18 13:15
Reason for closing:  Accepted
Additional comments about closing:  
r30380
Thomas Martitz commented on 2011-07-10 00:13

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.

Admin
Frank Gevaerts commented on 2011-07-29 21:34

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/

Thomas Martitz commented on 2011-07-30 14:09

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.

Thomas Martitz commented on 2011-08-03 21:02

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.

Thomas Martitz commented on 2011-08-04 11:22

Refined the patch a bit further, fixing a few bugs. This time I really added BUFLIB_CB_CANNOT_MOVE :)

Thomas Martitz commented on 2011-08-09 18:49

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.

Thomas Martitz commented on 2011-08-15 16:00

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.

sideral commented on 2011-08-22 17:35

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

Thomas Martitz commented on 2011-08-30 12:51

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

Available keyboard shortcuts

Tasklist

Task Details

Task Editing