Allocation improvements #32

isaachier · 2018-03-19T12:04:50Z

No description provided.

coveralls · 2018-03-19T12:17:01Z

Coverage remained the same at 100.0% when pulling 8e4b419 on isaachier:vla-fix into e545cf3 on uber:master.

dfellis

Not opposed to the PR or its intended changes, but there are other concerns mixed in with it and the changes to generateBaseCellNeighbors.c are simply incorrect.

dfellis · 2018-03-19T16:12:18Z

src/apps/benchmarks/benchmarkPolyfill.c

@@ -13,6 +13,7 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
+#include <string.h>


I don't see any code in here using string.h

memset for all of these comments.

dfellis · 2018-03-19T16:13:42Z

src/apps/filters/hexRange.c

@@ -30,13 +30,15 @@

 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>


Same here. printf is in stdio, I don't see why string was included.

dfellis · 2018-03-19T16:13:56Z

src/apps/filters/kRing.c

@@ -25,22 +25,22 @@

 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>


Please remove this.

dfellis · 2018-03-19T16:17:08Z

src/apps/miscapps/generateBaseCellNeighbors.c

@@ -237,18 +226,19 @@ void generate() {

    printf("const int baseCellNeighbors[NUM_BASE_CELLS][7] = {\n");
    for (int i = 0; i < NUM_BASE_CELLS; i++) {
-        char* neighborStrings[7] = {0, 0, 0, 0, 0, 0, 0};
+        printf("{");


Please undo the changes you made here. This approach will print garbage around the INVALID_BASE_CELL printout, instead of only printing that message when reaching that condition, so it will not generate the base cell neighbors code correctly, anymore.

Now I'm very confused. The difference between the old code and this is the removal of the generateBaseCellOutput, which simply formatted strings that are printed here. Instead, I used identical code here to write the digits/macro directly to stdout.

@dfellis Could you paste the output you are seeing or expecting? I ran this branch and got the same output as before.

My mistake on the reading of his changes. I did not run his branch.

dfellis · 2018-03-19T16:17:20Z

src/h3lib/lib/algos.c

@@ -22,6 +22,7 @@
 #include <math.h>
 #include <stdbool.h>
 #include <stdlib.h>
+#include <string.h>


Remove this.

isaacbrodsky · 2018-03-19T16:35:08Z

src/h3lib/lib/algos.c

@@ -90,9 +91,8 @@ int H3_EXPORT(maxKringSize)(int k) {
 */
 void H3_EXPORT(kRing)(H3Index origin, int k, H3Index* out) {
    int maxIdx = H3_EXPORT(maxKringSize)(k);
-    int* distances = calloc(maxIdx, sizeof(int));
+    int distances[maxIdx];


I was a little concerned this does not initialize distances. It might be that distances cannot be used before initializing. Not sure if you want to add a memset here for clarity.

Good catch. Was almost certain I followed them all with a memset.

isaacbrodsky · 2018-03-19T16:37:36Z

src/h3lib/lib/h3Index.c

        numRemainingHexes = compactableCount;
    }
-    // Final cleanup
-    free(remainingHexes);
-    free(hashSetArray);
    return 0;


It looks like this line is no longer reached in tests, perhaps because of the change around line 276. Could you change to preserve 100% coverage?

Sure. Will push fix for that now.

isaacbrodsky · 2018-03-19T16:40:06Z

src/apps/miscapps/generateBaseCellNeighbors.c

@@ -237,18 +226,19 @@ void generate() {

    printf("const int baseCellNeighbors[NUM_BASE_CELLS][7] = {\n");
    for (int i = 0; i < NUM_BASE_CELLS; i++) {
-        char* neighborStrings[7] = {0, 0, 0, 0, 0, 0, 0};
+        printf("{");


@dfellis Could you paste the output you are seeing or expecting? I ran this branch and got the same output as before.

isaachier · 2018-03-19T16:50:45Z

src/h3lib/lib/algos.c

@@ -741,7 +742,8 @@ void H3_EXPORT(polyfill)(const GeoPolygon* geoPolygon, int res, H3Index* out) {
    // This first part is identical to the maxPolyfillSize above.

    // Get the bounding boxes for the polygon and any holes
-    BBox* bboxes = calloc(sizeof(BBox), geoPolygon->numHoles + 1);
+    BBox bboxes[geoPolygon->numHoles + 1];
+    memset(bboxes, 0, sizeof(BBox));


This is a mistake. Let me fix now.

isaachier · 2018-03-19T16:51:59Z

src/h3lib/lib/algos.c

@@ -741,7 +742,8 @@ void H3_EXPORT(polyfill)(const GeoPolygon* geoPolygon, int res, H3Index* out) {
    // This first part is identical to the maxPolyfillSize above.

    // Get the bounding boxes for the polygon and any holes
-    BBox* bboxes = calloc(sizeof(BBox), geoPolygon->numHoles + 1);
+    BBox bboxes[geoPolygon->numHoles + 1];
+    memset(bboxes, 0, sizeof(bboxes));


isaachier · 2018-03-19T17:01:43Z

Think I addressed all the comments.

isaacbrodsky · 2018-03-19T17:12:06Z

I tested this on MSVC (Visual Studio 2017) and it seems it does not support variable length arrays. I would prefer the repo is limited to the subset of C99/C11 that MSVC supports to maximize how broadly it can be used.

nrabinowitz

One note, probably ignorable, otherwise LGTM.

nrabinowitz · 2018-03-19T17:13:49Z

src/h3lib/lib/linkedGeo.c

-            currentCoord = currentLoop->first;
-            while (currentCoord != NULL) {
+    bool skip = true;
+    for (LinkedGeoPolygon *currentPolygon = polygon, *nextPolygon;


Nit: I believe our style in this project is LinkedGeoPolygon* currentPolygon, can we use that here? Or would that require LinkedGeoPolygon* currentPolygon, LinkedGeoPolygon* nextPolygon?

You mean the pointer alignment? Ya it's a problem. clang-format isn't even fixing it. The only alternative is LinkedGeoPolygon* nextPolygon; followed by for (LinkedGeoPolygon* currentPolygon = ...). I once answered a question about this on StackOverflow here: https://stackoverflow.com/a/16490876/1930331.

isaacbrodsky

*Requested change is discussing what features of C we want to require.

isaacbrodsky · 2018-03-22T02:41:21Z

cmake/CheckAlloca.cmake

@@ -0,0 +1,21 @@
+if(__check_alloca)


Please include the license header in all source files.

isaacbrodsky · 2018-03-22T02:42:15Z

src/h3lib/include/stackAlloc.h

@@ -0,0 +1,44 @@
+#ifndef STACKALLOC_H


Please also include a @file/@brief comment describing the purpose of this file.

isaacbrodsky · 2018-03-22T02:44:34Z

src/apps/miscapps/generateBaseCellNeighbors.c

-               neighborStrings[0], neighborStrings[1], neighborStrings[2],
-               neighborStrings[3], neighborStrings[4], neighborStrings[5],
-               neighborStrings[6], i,
+        printf("}, /* base cell %d%s */\n", i,


Since the formatting of the comments changes, could you rerun generateBaseCellNeighbors and update baseCells.c?

Sure thing.

isaacbrodsky · 2018-03-22T02:54:56Z

I successfully built and tested this on MSVC :) - #34 will make that automated.

isaachier · 2018-03-22T02:59:04Z

Luckily even MSVC has the alloca function. It's a bit ugly but this should definitely allow some form of VLA on all popular platforms.

isaacbrodsky · 2018-03-22T17:32:23Z

src/apps/miscapps/generateBaseCellNeighbors.c

+    reentrant_gmtime(&tmStruct, &rawTime);
+    const char licenseFormat[] =
+        "/*\n"
+        " * Copyright 2016-%d Uber Technologies, Inc.\n"


This seems like a lot of work just to automatically format the date. I don't think the parts of the baseCells.c file unrelated to the baseCellNeighbors/baseCellNeighbor60CCWRots should be generated. (Those two were the only parts of the file that were generated from this - maybe those two could be split off to their own, generated file.)

Could you revert this change, and we can revisit it in another issue or PR?

jogly

lgtm

Allocation improvements

isaachier added 3 commits March 19, 2018 08:02

Handle allocation on stack when possible using VLA

8417011

Guard dynamic allocations with assertions

de204e9

Fix includes

c43c15a

Remove unused variable

4421816

dfellis requested changes Mar 19, 2018

View reviewed changes

isaachier mentioned this pull request Mar 19, 2018

Why C11? #33

Closed

isaacbrodsky reviewed Mar 19, 2018

View reviewed changes

Zero uninitialized distances array

d67630b

isaachier commented Mar 19, 2018

View reviewed changes

Fix mistake in initializing bboxes memory

d2c93bf

isaachier commented Mar 19, 2018

View reviewed changes

Use break to guarantee 100% coverage

e21eda7

dfellis approved these changes Mar 19, 2018

View reviewed changes

nrabinowitz approved these changes Mar 19, 2018

View reviewed changes

isaacbrodsky requested changes Mar 19, 2018

View reviewed changes

isaachier added 11 commits March 21, 2018 15:10

h3 depends on m, h3 executables depend on h3

08c590e

Use CMake to test for VLA and alloca

c94e4ec

Use compatibility macro instead of VLA

05df279

Apply fix to h3Index

c4fad6e

Fix warnings

93e5bc1

Make sure stack allocations are not empty

9a1ac18

Fix VLA test

3a9abd1

Fix compiler detection

d2f4c1c

Fix test cases for Windows

155346a

Fix test command

c76f7ff

Fix valgrind wrapper, fix vla_test again

583c63d

isaacbrodsky reviewed Mar 22, 2018

View reviewed changes

isaachier added 2 commits March 22, 2018 07:53

Work on file generator

3e6880a

Minor changes

472a799

isaacbrodsky reviewed Mar 22, 2018

View reviewed changes

Remove generator fixes and revert comment style

8e4b419

isaacbrodsky approved these changes Mar 22, 2018

View reviewed changes

jogly approved these changes Mar 23, 2018

View reviewed changes

isaacbrodsky merged commit f87862a into uber:master Mar 24, 2018

isaachier deleted the vla-fix branch August 5, 2018 22:49

mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022

Merge pull request uber#32 from isaachier/vla-fix

5e54b36

Allocation improvements

Allocation improvements #32

Allocation improvements #32

Conversation

isaachier commented Mar 19, 2018

coveralls commented Mar 19, 2018 • edited Loading

dfellis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier commented Mar 19, 2018

isaacbrodsky commented Mar 19, 2018

nrabinowitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacbrodsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacbrodsky commented Mar 22, 2018

isaachier commented Mar 22, 2018

Choose a reason for hiding this comment

jogly left a comment

Choose a reason for hiding this comment

coveralls commented Mar 19, 2018 •

edited

Loading