annacrombie / plot

plot on the command line
MIT License
113 stars 9 forks source link

WIP: refactor: migrate from stack allocations to dynamic allocations #12

Closed AdrianDC closed 3 years ago

AdrianDC commented 3 years ago

A refactoring based on @holmanb initial dynamic allocations commits, with:

Only the earliest two commits are on review in the PR, until the other ones are merged.

Further work must be done to simplify and cleanup the sources.

annacrombie commented 3 years ago

First off, this was actually the original way plot was implemented, but I refactored it last april (https://github.com/annacrombie/plot/commit/e4b2d91173f531ddf30b9ada540887c155c8562b).

My reasons to use only stack allocation were robustness and speed. If you never call alloc, you can't have a memory leak. I don't really see a reason to switch back to heap allocation, since plot doesn't need much memory anyway. Please let me know your thoughts though

holmanb commented 3 years ago

My original reasoning for doing this was that when growing the plot to larger fixed size bounds (see #11), the stack starts to grow and I was having issues with stack sizes if I remember correctly.

In my project I'm temporarily using the default plot bounds behaviour, but I do plan on eventually switching back to the -b option for fixed sizes and I expect the bounds to require larger values (10k, maybe higher). I honestly didn't understand the plot code very well when I started looking for a way to get larger fixed sizes, so I'm sure somebody with a better understanding of plot's code might have better ideas than me.

annacrombie commented 3 years ago

The bounds represent the minimum and maximum values in the data so that it can be scaled to the correct range (based on the plot dimensions). Modifying values of bounds should not affect the memory usage. The only thing that would require a lot of memory would be making the plot dimensions very large. Since this tool is meant to format output for display on a terminal, which almost never has a very big resolution (my full-screen terminal is only 170x48), I feel comfortable putting a hard limit on the plot dimensions. Users who really need huge text plots can feel free to modify the MAX_HEIGHT / MAX_WIDTH an recompile, possibly even allocating the plot struct dynamically if stack space is an issue.

AdrianDC commented 3 years ago

Will close the PR within the next weeks.