Rockbox

Tasklist

FS#12378 - Remove unused code and data

Attached to Project: Rockbox
Opened by Boris Gjenero (dreamlayers) - Wednesday, 09 November 2011, 18:36 GMT
Task Type Patches
Category Operating System/Drivers
Status New
Assigned To No-one
Operating System All players
Severity Low
Priority Low
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

I recently tried to compile Rockbox with binutils-2.21.1 and gcc-4.6.2 with link-time optimization (LTO). I didn't produce copy that ran correctly, but the resulting rockbox.map was useful for finding unused code and data. LTO can also remove symbols for other reasons, so this list was created by finding which of the removed symbols only occur on one line in one C file. I only compared 5G iPod and Archos v2 Recorder builds.

Here are the small memory savings with r30944, using the normal toolchain:
For the 5G iPod:
< Binary size: 687368
< Actual size: 687360
< RAM usage: 1199984
---
> Binary size: 686824
> Actual size: 686816
> RAM usage: 1199440

For the Archos v2 Recorder:
< Binary size: 301984
< Actual size: 301960
< RAM usage: 640700
---
> Binary size: 301328
> Actual size: 301304
> RAM usage: 640044

What follows is the output from grepping through the r30944 code for these symbols. For more information, see the attached patch.

apps/playback.c:bool audio_buffer_state_trashed(void)
apps/playback.h:bool audio_buffer_state_trashed(void);

apps/radio/radio.h:struct gui_wps *fms_get(enum screen_type screen);
apps/radio/radio_skin.c:struct gui_wps *fms_get(enum screen_type screen)

apps/gui/list.c:bool gui_synclist_item_is_onscreen(struct gui_synclist *lists,
apps/gui/list.h:extern bool gui_synclist_item_is_onscreen(struct gui_synclist *lists,

firmware/export/mascodec.h:int mas_default_read(unsigned short *buf);
firmware/target/sh/archos/mascodec-archos.c:int mas_default_read(unsigned short *buf)

firmware/export/mascodec.h:int mas_direct_config_read(unsigned char reg);
firmware/target/sh/archos/mascodec-archos.c:int mas_direct_config_read(unsigned char reg)

firmware/enc_base.c:const unsigned long mp3_enc_sampr[MP3_ENC_NUM_SAMPR] =
firmware/export/enc_base.h:extern const unsigned long mp3_enc_sampr[MP3_ENC_NUM_SAMPR];

firmware/export/mp3_playback.h:long mp3_get_playtime(void);
firmware/target/sh/archos/audio-archos.c:long mp3_get_playtime(void)

apps/playlist_viewer.c:MENUITEM_FUNCTION(save_playlist_item, 0, ID2P(LANG_SAVE_DYNAMIC_PLAYLIST),

apps/gui/skin_engine/skin_display.c:void skin_statusbar_changed(struct gui_wps *skin)
apps/gui/skin_engine/skin_engine.h:void skin_statusbar_changed(struct gui_wps*);

apps/tagcache.c:long tagcache_get_commitid(void)

apps/tagcache.c:long tagcache_get_serial(void)
apps/tagcache.h:long tagcache_get_serial(void);

apps/tagcache.c:bool tagcache_is_busy(void)
apps/tagcache.h:bool tagcache_is_busy(void);

apps/tagcache.c:bool tagcache_is_ramcache(void)
apps/tagcache.h:bool tagcache_is_ramcache(void);

apps/tree.c:void tree_drawlists(void)
apps/tree.h:void tree_drawlists(void);

firmware/export/usb.h:void usb_storage_try_release_storage(void);
firmware/usbstack/usb_storage.c:void usb_storage_try_release_storage(void)
This task depends upon

Comment by Torne Wuff (torne) - Thursday, 10 November 2011, 11:04 GMT
Can't we just remove all the unused symbols by tree shaking? Add -ffunction-sections and -fdata-sections to compile flags; add --gc-sections to linker flags.
Comment by Boris Gjenero (dreamlayers) - Thursday, 10 November 2011, 17:00 GMT
Thanks for telling me about those options. I just tried them out on an r30948 5G iPod build.

-fdata-sections actually creates a larger binary that uses more memory, probably via padding for alignment. I guess it's only useful for finding unused data to remove manually. It just removed 4 symbols: afmt_rec_format, attenuation, mp3_enc_sampr, save_playlist_item

-ffunction-sections is great! I had to add KEEP( ) around *(.vectors) in ram.link, but once that was done I got a smaller binary that seems to work fine. Here's the memory savings:
< Binary size: 687384
< Actual size: 687376
< RAM usage: 1200000
---
> Binary size: 685256
> Actual size: 685248
> RAM usage: 1197872

I'm attaching a list of removed functions. I searched through the code for some of them and found that in many cases they're only used on certain targets. It would be possible to remove them via preprocessor conditionals, but that could get messy. Use of -ffunction-sections seems like a good idea. What are others' thoughts on this?

Note that what I posted first (remove-unused.patch) is code and data that's not used in all of Rockbox. It's easy and simple to remove. I'm just not sure if others agree with its removal.
Comment by Michael Sevakis (MikeS) - Thursday, 10 November 2011, 19:46 GMT
I'd keep enc_base.c/h routines but "#if 0"'ed rather than deleted for the sake of reference.
Comment by Boris Gjenero (dreamlayers) - Thursday, 10 November 2011, 20:01 GMT
Agreed. It's also probably better to "#if 0" the unused mas codec functions.
Comment by Torne Wuff (torne) - Friday, 11 November 2011, 11:06 GMT
Ah, yes, data sections will be some nontrivial overhead for us because we make heavy use of sub-4-byte aligned types in global variables; for most non-embedded code it generally costs virtually nothing. We might be able to do some trickery by making the toolchain generate lower-aligned sections but it's probably not worth bothering. Function sections should be "free" because ARM code is 4-byte aligned anyway and so functions are already padded.

For code that's literally never used by any target, we should remove/#if 0 it, yes, but I think we should probably just add -ffunction-sections and -Wl,--gc-sections to the normal build; that way the tree shaking will just remove any code that's not needed by a given build config. The only real cost to it is having to KEEP() symbols that are not referred to from the entry point, but that should only be the vectors as you noted. (technically it makes linking slower, bur our binaries are so tiny by the toolchain's standards that I doubt anyone will notice.)
Comment by Boris Gjenero (dreamlayers) - Thursday, 17 November 2011, 04:49 GMT
Here's a patch which uses "#if 0" to exclude some unneeded but interesting and potentially useful functions. It's confined to just Archos-specific code. I'm wondering if it's ok to commit.

I'm also attaching a file showing symbols unused in some or all HWCODEC targets in r31001. It's based on automated comparison of map files of standard builds with builds using -ffunction-sections and -fdata-sections. Unlike with the 5G iPod, the linker script already contains the needed KEEP directives. I tested the V2 Recorder build.

Amiconn pointed out that sh instructions are only 2 bytes, and so 4 byte alignment wastes on average one byte per function. Nevertheless -ffunction-sections (without -fdata-sections) saved about 2KB for the v2 Recorder build. Yes, proper conditionals could be better, but does anyone really want to go through the list of symbols I posted, and add proper conditionals, considering all the various targets and bootloaders?

Edit: Updated files, fixing an error in hwcodec_unused_symbols.txt and adding a few more conditionals. The list can still be misleading when a function is inlined for usage in the same file, and not used from other files.

Edit: Archos-specific unused functions within the target tree were dealt with in r31043.
Comment by Boris Gjenero (dreamlayers) - Saturday, 10 December 2011, 14:54 GMT
Here is a patch to allow building with -ffunction-sections -fdata-sections in GCCOPTS and -Wl,--gc-sections in GLOBAL_LDOPTS. It doesn't actually add those switches.

The patch adds KEEP() to vectors everywhere. I'm not certain that it's wise to touch linker scripts that aren't used for regular target and bootloader builds, but KEEP() shouldn't cause any harm. It also adds required wildcards to firmware/rom.lds, which is used for iriver h1x0 ROM builds and Archos RomBox. It doesn't add other wildcards, because things that are explicitly put into a particular section won't end up in a subsection.

The attached csv shows what is removed. You can also see it at Google Docs here https://docs.google.com/spreadsheet/ccc?key=0AsEpnuZKewlIdHMteloydnV2eERmWjZsTV9OT2w0UWc

(Comment edited for r31343.)

Loading...