Use calloc for potentially large user-defined inputs

[isaachier]

No description provided.

[coveralls]

Coverage remained the same at 100.0% when pulling 70a099f on isaachier:calloc-large-arrays into e506b81 on uber:master.

[dfellis]

Thank you so much for doing this, @isaachier 😃

[isaachier]

Np thanks for pointing this out @dfellis. 😄

[nrabinowitz]

LGTM, are we sure it covers all cases? I'd imagine we'd be most concerned with arrays of hexagons.

[nrabinowitz]

I don't think this one's a risk - it scales with the number of polygons, not the number of hexagons, and should be fine with stack allocation.

[dfellis]

I say better safe than sorry here. What if someone provides a geofence of the US, but just does it lazily by shoving in all of the county geofences in a huge array? (Or attempting to guarantee exact set matching if they need national and county level hexagon sets as a more rational explanation.)

[nrabinowitz]

Point taken. I didn't realize we were removing all instances of STACK_ARRAY_CALLOC, in which case sure. No real preference here.

[dfellis]

@nrabinowitz

LGTM, are we sure it covers all cases? I'd imagine we'd be most concerned with arrays of hexagons.

So I'm pretty sure @isaachier has removed all uses of STACK_ARRAY_CALLOC from the actual library. It's now used only in some of the apps.

damocles@Talyn:~/oss/h3/src/h3lib(master)$ git grep STACK_ARRAY_CALLOC
include/stackAlloc.h:#define STACK_ARRAY_CALLOC(type, name, numElements)        \
include/stackAlloc.h:#define STACK_ARRAY_CALLOC(type, name, numElements)            \
include/stackAlloc.h:#define STACK_ARRAY_CALLOC(type, name, numElements)           \
lib/algos.c:    STACK_ARRAY_CALLOC(int, distances, maxIdx);
lib/algos.c:    STACK_ARRAY_CALLOC(BBox, bboxes, geoPolygon->numHoles + 1);
lib/h3Index.c:    STACK_ARRAY_CALLOC(H3Index, remainingHexes, numHexes);
lib/h3Index.c:    STACK_ARRAY_CALLOC(H3Index, hashSetArray, numHexes);
lib/h3Index.c:        STACK_ARRAY_CALLOC(H3Index, compactableHexes, maxCompactableCount);

Two uses in algos.c and three in h3Index.c, and that's how many changes I count in his PR. :)

So the only stack allocations are now constant amounts of memory, which is much safer. (It would be nice if we could eliminate all arbitrary memory allocations from the library and let the user decide on stack vs heap allocation themselves, but that would require some API changes and some awkward API usage patterns, where some blocks of memory are requested to be freed immediately by the user of the API.)

[isaachier]

@dfellis many C libraries let you override the default allocator. Here's an example from one of my favorite C libraries, Jansson:

https://github.com/akheron/jansson/blob/aed855e6920923898b94a1b922fbace27a34ddf2/src/jansson.h#L355-L361:

/* custom memory allocation */

typedef void *(*json_malloc_t)(size_t);
typedef void (*json_free_t)(void *);

void json_set_alloc_funcs(json_malloc_t malloc_fn, json_free_t free_fn);
void json_get_alloc_funcs(json_malloc_t *malloc_fn, json_free_t *free_fn);

It just stores a custom malloc and free in static variables and uses those across the board. That allows users to implement custom allocators.

[dfellis]

Something to consider, but that proposed implementation wouldn't be threadsafe if you want to choose the allocator based on the function call or data volume. Go too far and we'll start reimplementing C++. 😉

[isaachier]

You can always use pthreads... but I get the point. Globals aren't straightforward.