From 1ccb51460c32cdb87d6f0e17a7c7a3145ca47417 Mon Sep 17 00:00:00 2001 From: twinaphex Date: Fri, 14 Aug 2020 17:00:50 +0200 Subject: [PATCH] Do some struct ordering for better alignment, and add CODING-GUIDELINES --- CODING-GUIDELINES | 55 +++++++++++++++++++++++++++++++++++++++++++++++ playlist.c | 21 +++++++++--------- playlist.h | 6 +++--- 3 files changed, 69 insertions(+), 13 deletions(-) create mode 100644 CODING-GUIDELINES diff --git a/CODING-GUIDELINES b/CODING-GUIDELINES new file mode 100644 index 0000000000..d6a5a0edc8 --- /dev/null +++ b/CODING-GUIDELINES @@ -0,0 +1,55 @@ +Struct ordering +--------------- + +For POD-types, try to order structs as follows (first to last): + +* long double (16 bytes [64bit x86], 12 bytes [32bit x86], 8 bytes) +* double (8 bytes) +* int64_t (8 bytes, 8 bytes [32bit ARM], 4 bytes [32bit x86]) +* uint64_t (4 bytes [32bit], 8 bytes [32bit ARM], 8 bytes [64bit]) +* pointer (4 bytes [32bit], 8 bytes [64bit]) +* intptr_t (4 bytes [32bit], 8 bytes [64bit]) +* uintptr_t (4 bytes [32bit], 8 bytes [64bit]) +* ptrdiff_t (4 bytes [32bit], 8 bytes [64bit]) +* ssize_t (4 bytes [32bit], 8 bytes [64bit]) +* size_t (4 bytes [32bit], 8 bytes [64bit]) +* long (4 bytes [64bit Win], 8 bytes [64bit non-Win], 4 bytes [32bit]) +* int32_t (4 bytes) +* float (4 bytes) +* int (4 bytes) +* enum (4 bytes) +* int16_t (2 bytes) +* char (1 byte) +* bool (1 byte) + +Struct members should be sorted by alignment. Therefore, structs +should be sorted by the largest type inside them. + +For example, take a struct like this: + +typedef struct +{ + size_t capacity; + bool old_format; + bool compress; + bool fuzzy_archive_match; + bool autofix_paths; + char path[PATH_MAX_LENGTH]; + char base_content_directory[PATH_MAX_LENGTH]; +} playlist_config_t; + +size_t has the biggest alignment here, so 'struct playlist_config_t' inside a struct should come before or after size_t. + +*** BEST PRACTICES *** + +* If we have pointers and size variable pairs, it's best to interleave them to increase the probability they go in the same cacheline. It also makes the code more readable, that these two variables are connected. + +Example: + +struct a +{ + char* b; + size_t b_len; + char* c; + size_t c_len; +}; diff --git a/playlist.c b/playlist.c index fa27bf10ce..da412f80d5 100644 --- a/playlist.c +++ b/playlist.c @@ -54,22 +54,23 @@ struct content_playlist { - bool modified; - bool old_format; - bool compressed; - bool cached_external; + char *default_core_path; + char *default_core_name; + char *base_content_directory; + + struct playlist_entry *entries; + + playlist_config_t config; enum playlist_label_display_mode label_display_mode; enum playlist_thumbnail_mode right_thumbnail_mode; enum playlist_thumbnail_mode left_thumbnail_mode; enum playlist_sort_mode sort_mode; - char *default_core_path; - char *default_core_name; - char *base_content_directory; - - struct playlist_entry *entries; - playlist_config_t config; + bool modified; + bool old_format; + bool compressed; + bool cached_external; }; typedef struct diff --git a/playlist.h b/playlist.h index 88aa5fa190..e3f27a91b0 100644 --- a/playlist.h +++ b/playlist.h @@ -99,7 +99,6 @@ struct playlist_entry char *runtime_str; char *last_played_str; struct string_list *subsystem_roms; - enum playlist_runtime_status runtime_status; unsigned runtime_hours; unsigned runtime_minutes; unsigned runtime_seconds; @@ -112,19 +111,20 @@ struct playlist_entry unsigned last_played_hour; unsigned last_played_minute; unsigned last_played_second; + enum playlist_runtime_status runtime_status; }; /* Holds all configuration parameters required * when initialising/saving playlists */ typedef struct { - char path[PATH_MAX_LENGTH]; - char base_content_directory[PATH_MAX_LENGTH]; size_t capacity; bool old_format; bool compress; bool fuzzy_archive_match; bool autofix_paths; + char path[PATH_MAX_LENGTH]; + char base_content_directory[PATH_MAX_LENGTH]; } playlist_config_t; /* Convenience function: copies specified playlist