Generate polygonToCells benchmark with Natural Earth country geometry #795

nrabinowitz · 2023-11-01T00:05:50Z

Adds a script and CMake targets to download relatively simple geometry for all countries and create a benchmark file. The benchmark currently runs both old and new algos for comparison; my local output:

Res 0	-- polygonToCells_AllCountries: 195351.800000 microseconds per iteration (5 iterations)
	-- polygonToCells_AllCountries: 35108.600000 microseconds per iteration (5 iterations)
Res 1	-- polygonToCells_AllCountries: 329267.000000 microseconds per iteration (5 iterations)
	-- polygonToCells_AllCountries: 18723.800000 microseconds per iteration (5 iterations)
Res 2	-- polygonToCells_AllCountries: 258062.200000 microseconds per iteration (5 iterations)
	-- polygonToCells_AllCountries: 65198.200000 microseconds per iteration (5 iterations)
Res 3	-- polygonToCells_AllCountries: 431086.600000 microseconds per iteration (5 iterations)
	-- polygonToCells_AllCountries: 280404.000000 microseconds per iteration (5 iterations)
Res 4	-- polygonToCells_AllCountries: 2043707.600000 microseconds per iteration (5 iterations)
	-- polygonToCells_AllCountries: 1464811.400000 microseconds per iteration (5 iterations)
Res 5	-- polygonToCells_AllCountries: 70389617.800000 microseconds per iteration (5 iterations)
	-- polygonToCells_AllCountries: 8704045.800000 microseconds per iteration (5 iterations)

At present, the CMake setup is a little awkward, as I wasn't sure how to add targets before you generate the source. To run the benchmark, you need:

cmake ..
make generate-country-benchmark
cmake ..
make bench_benchmarkCountries

I might be able to consolidate those steps, but I also want to be able to hand-edit the output and easily make/run again to try different benchmarks. Would love advice from someone more adept at CMake.

coveralls · 2023-11-01T00:10:46Z

coverage: 98.784%. remained the same
when pulling da28757 on nrabinowitz:updated-polyfill-countries
into 5d7c1c1 on uber:master.

isaacbrodsky · 2023-11-01T01:55:44Z

https://github.com/uber/h3/actions/runs/6713819602/job/18246045886?pr=795 This looks like an unrelated fuzzer timeout

isaacbrodsky · 2023-11-01T01:56:24Z

scripts/make_countries.js

+function formatGeoLoop(loop) {
+  return `{
+    .numVerts = ${loop.length},
+    .verts = (LatLng[]){


Suggested change

.verts = (LatLng[]){

.verts = {

Can we elide this cast?

No, the casts to LatLng[] and GeoLoop[] seem to be necessary to get the compiler to understand the nested arrays. Is there any significant downside to including the cast?

scripts/make_countries.js

isaacbrodsky · 2023-11-01T01:58:32Z

.gitignore

@@ -105,3 +106,4 @@ KML/res*centers.kml

 # Generated files
 src/h3lib/include/h3api.h
+src/apps/benchmarks/benchmarkCountries.c


Maybe this could be put into a different directory to indicate it's generated?

I guess it can go into the build directory - the nice thing about having it here is that it's a bit easier to edit, e.g. if your IDE is eliding build

My understanding is that like h3api.h, it would get placed in the build directory if you are doing an out-of-source build, and this was just in case you're doing an in-source build.

isaacbrodsky · 2023-11-01T01:59:36Z

CMakeLists.txt

+    if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/src/apps/benchmarks/benchmarkCountries.c")
+       add_h3_benchmark(benchmarkCountries src/apps/benchmarks/benchmarkCountries.c)
+    endif()


Maybe this should be an optional compilation step and require the user specify -DBUILD_COUNTRIES_BENCHMARK=ON? Then you would need to add a dependency from the benchmarkCountries target to the generate-country-benchmark target, e.g. with add_dependency

The thing I struggled with here is that I want to be able to edit the generated files and then re-run, e.g. to benchmark a single country or change the res. If I add the generate-country-benchmark target as a dependency, I think it'll run every time I run make bench_benchmarkCountries, overriding local changes. Open to ideas here.

Perhaps this can be done with BYPRODUCTS? https://cmake.org/cmake/help/latest/command/add_custom_target.html I can try to look into coding that later

Edit: The docs say you need https://cmake.org/cmake/help/latest/command/add_custom_command.html#command:add_custom_command to tell CMake you are generating a file in the build system.

isaacbrodsky · 2023-11-01T02:00:07Z

CMakeLists.txt

@@ -577,6 +577,16 @@ if(BUILD_BENCHMARKS)
    if(ENABLE_REQUIRES_ALL_SYMBOLS)
        add_h3_benchmark(benchmarkPolygon src/apps/benchmarks/benchmarkPolygon.c)
    endif()
+
+    # Country benchmark: Downloads country geometry and generates the benchmark file
+    add_custom_target(generate-country-benchmark


Suggested change

add_custom_target(generate-country-benchmark

add_custom_target(generate_country_benchmark

Will rename, thanks

scripts/make_countries.js

isaacbrodsky · 2023-11-02T13:48:53Z

scripts/make_countries.js

+    }
+  });
+
+  BENCHMARK(polygonToCells_AllCountries, 5, {


Suggested change

BENCHMARK(polygonToCells_AllCountries, 5, {

BENCHMARK(polygonToCellsExperimental_AllCountries, 5, {

Or perhaps add 1 (previous)/2 (experimental) after both names

…nd turn on in CI

Nick Rabinowitz added 4 commits October 31, 2023 10:34

Add all-countries benchmark, with script

ae28276

Use better GeoJSON source, enumerate polygons and verts

2593b0d

Benchmark old/new against each other

af42556

Rename generate target'

26dd346

nrabinowitz requested review from dfellis, ajfriend and isaacbrodsky November 1, 2023 00:06

isaacbrodsky reviewed Nov 1, 2023

View reviewed changes

Code review updates

20793d2

isaacbrodsky reviewed Nov 2, 2023

View reviewed changes

change per review, change cmake

0465733

isaacbrodsky approved these changes Nov 2, 2023

View reviewed changes

add documentation comment on BUILD_COUNTRY_BENCHMARKS, default off, a…

da28757

…nd turn on in CI

nrabinowitz merged commit 6c1b003 into uber:master Nov 3, 2023
33 checks passed

nrabinowitz deleted the updated-polyfill-countries branch November 3, 2023 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate polygonToCells benchmark with Natural Earth country geometry #795

Generate polygonToCells benchmark with Natural Earth country geometry #795

nrabinowitz commented Nov 1, 2023

coveralls commented Nov 1, 2023 •

edited

Loading

isaacbrodsky commented Nov 1, 2023

isaacbrodsky Nov 1, 2023

nrabinowitz Nov 1, 2023

isaacbrodsky Nov 1, 2023

nrabinowitz Nov 1, 2023

isaacbrodsky Nov 2, 2023

isaacbrodsky Nov 1, 2023

nrabinowitz Nov 1, 2023

isaacbrodsky Nov 2, 2023 •

edited

Loading

isaacbrodsky Nov 1, 2023

nrabinowitz Nov 1, 2023

isaacbrodsky Nov 2, 2023 •

edited

Loading

	add_custom_target(generate-country-benchmark
	add_custom_target(generate_country_benchmark

	BENCHMARK(polygonToCells_AllCountries, 5, {
	BENCHMARK(polygonToCellsExperimental_AllCountries, 5, {

Generate polygonToCells benchmark with Natural Earth country geometry #795

Generate polygonToCells benchmark with Natural Earth country geometry #795

Conversation

nrabinowitz commented Nov 1, 2023

coveralls commented Nov 1, 2023 • edited Loading

isaacbrodsky commented Nov 1, 2023

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 Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacbrodsky Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 1, 2023 •

edited

Loading

isaacbrodsky Nov 2, 2023 •

edited

Loading

isaacbrodsky Nov 2, 2023 •

edited

Loading