a-sevin
commented
6 years ago Sorry for the late answer...
Fully agree with your point [1]
For the [2&3], we can see that as a singleton, but I prefer an implementation where data, data.image and data.variable arrays are allocated at the run-time in the main function (configured with an INI-file or XML-file for example) and pass the DATA structure as reference argument to functions who need to handle it...
But your proposition takes the advantage of a fast solution.
oguyon
IMPORTANT: I am proposing the following changes to the data structure in milk (+cacao, coffee). Please read carefully and let me know if you disagree.
[1] Remove default attribute ((packed)) for IMAGE_METADATA, IMAGE, EVENT_UI8_UI8_UI16_UI8, and EVENT_UI8_UI8_UI16_UI8.
I propose to enable these with an option specified at compile time, but have the default behavior to not pack the data. Why ? Code will run faster and be more portable to non gcc compilers. Compiler can do more optimization. I ran into a nasty memory alignment bug when compiling with pgcc with -fast option which I do not yet fully understand, but clearly due to this packing. Removing the packing solved the issue. Read this (pointed out by Frantz): https://stackoverflow.com/questions/8568432/is-gccs-attribute-packed-pragma-pack-unsafe and this (pointed out by Jared): http://www.catb.org/esr/structure-packing/ Why not ? Very small loss in memory usage efficiency. Possible impact on low-level reading of fields in structure as pointer offsets may be compiler-optimized (note: this should be addressed by other approaches, and milk could print out pointer offsets to a file for others to read)
[2] Simplify data, and new locations for definition/declaration
data is currently a variable of type DATA (using typedef to a structure). Since we only have one instance of this DATA type variable, I propose to get rid of the typedef. I also propose to declare the data variable as extern in CLIcore.h which is already included in all source files requiring its use.
The proposed change would have in CLIcore.h: struct DATA { ....}; extern struct DATA data;
data will be defined in CLImain.c: struct DATA data;
.. and will no longer need to be declared in other files (part of included CLIcore.h)
[3] Implement static allocation of data.image and data.variable arrays, provide use with option to set smaller initial image and variable array sizes.
The current scheme is to have a dynamic allocation (default: 5000 instances) and grow it with reallocs when more needed. This works, but is not optimal is speed (compiler can better optimize with statically allocated arrays), and in some cases, we need to support smaller sized arrays for faster I/O. I propose to :