Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experiment on pulling NVTX3 via CPM #1002

Draft
wants to merge 79 commits into
base: branch-23.08
Choose a base branch
from

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Mar 20, 2023

Description

This PR experiments pulling nvtx3 via CPM.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@isVoid
Copy link
Contributor Author

isVoid commented Apr 18, 2023

This PR require adding nvtx dependency to conda environment.

@@ -198,6 +210,7 @@ target_compile_definitions(cuspatial PUBLIC "SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_${

# Specify the target module library dependencies
target_link_libraries(cuspatial PUBLIC cudf::cudf)
target_link_libraries(cuspatial PRIVATE nvtx3-cpp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a PUBLIC dependency not a PRIVATE one. The nvtx3 headers are included by the cuspatial public headers and therefore consumers of cuspatial will also need nvtx3.

This will resolve the test compilation failures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expected to need PUBLIC here and tried it but it caused another issue. @isVoid Can you provide the logs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nvtx3 project doesn't have have install / export rules for the targets they create it looks like. So you would get errors when trying to export the cuspatial targets as they now depend on targets that have no way to be exported.

What we should do is specify DOWNLOAD_ONLY when getting nvtx3 and directly including nvtxImportedTargets.cmake ( https://github.com/NVIDIA/NVTX/blob/v3.1.0-c-cpp/CMakeLists.txt#L33 ).

We will need to also setup install rules for this file and the nvtx3 headers, plus a custom FINAL_CODE_BLOCK I expect for the rapids_export.

I expect that this is why previously cuspatial copied nvtx3 directly, as it dramatically simplified the build-system.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly what we ran into. We tried setting it to PUBLIC and used rapids_cpm_find.

The nvtx3 project doesn't have have install / export rules for the targets they create it looks like.

Should we look into contributing to nvtx3 to provide install / export rules?

Copy link
Member

@harrism harrism Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we look into contributing to nvtx3 to provide install / export rules?

Yes but don't hold your breath for it to be put in a release anytime soon. It took them about 2 years to release the last merged feature I was waiting on. So you will probably need to take Rob's DOWNLOAD_ONLY suggestion as well.

@isVoid isVoid changed the title Add pairwise_point_polygon_distance benchmark Experiment on pulling NVTX3 via CPM May 9, 2023
@github-actions github-actions bot added the Python Related to Python code label May 10, 2023
rapids-bot bot pushed a commit that referenced this pull request May 23, 2023
This PR separates the `pairwise_point_polygon_distance` benchmark portion of PR #1002. While that PR is only left for nvtx3 experiments.

# Original PR description:

This PR adds pairwise point polygon distance benchmark. Depends on #998

Point-polygon distance performance can be affected by many factors, because the geometry is complex in nature. I benchmarked these questions:
1. How does the algorithm scales with simple multipolygons?
2. How does it scales with complex multipolygons?

## How does the algorithm scales with simple multipolygons?
The benchmark uses the most simple multipolygon, 3 sides per polygon, 0 hole and 1 polygon per multipolygon.

Float32
| Num   multipolygon | Throughput   (#multipolygons / s) |
| --- | --- |
| 1 | 28060.32971 |
| 100 | 2552687.469 |
| 10000 | 186044781 |
| 1000000 | 1047783101 |
| 100000000 | 929537385.2 | 

Float64
| Num   multipolygon | Throughput   (#multipolygons / s) |
| --- | --- |
| 1 | 28296.94817 |
| 100 | 2491541.218 |
| 10000 | 179379919.5 |
| 1000000 | 854678939.9 |
| 100000000 | 783364410.7 |

![image](https://user-images.githubusercontent.com/13521008/226502300-c3273d80-5f9f-4d53-b961-a24e64216e9b.png)

The chart shows that with simple polygons and simple multipoint (1 point per multipoint), the algorithm scales pretty nicely. Throughput is maxed out at near 1M pairs.

## How does the algorithm scales with complex multipolygons?

The benchmark uses a complex multipolygon, 100 edges per ring, 10 holes per polygon and 3 polygons per multipolygon.

float32
Num   multipolygon | Throughput   (#multipolygons / s)
-- | --
1000 | 158713.2377
10000 | 345694.2642
100000 | 382849.058

float64
Num   multipolygon | Throughput   (#multipolygons / s)
-- | --
1000 | 148727.1246
10000 | 353141.9758
100000 | 386007.3016

![image](https://user-images.githubusercontent.com/13521008/226502732-0d116db7-6257-4dec-b170-c42b30df9cea.png)

The algorithm reaches max throughput at near 10K pairs. About 100X lower than the simple multipolygon example.

Authors:
  - Michael Wang (https://github.com/isVoid)
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Mark Harris (https://github.com/harrism)

URL: #1131
@harrism
Copy link
Member

harrism commented May 31, 2023

@isVoid is this ready to go into 23.06?

@isVoid isVoid changed the base branch from branch-23.06 to branch-23.08 May 31, 2023 22:24
@isVoid isVoid marked this pull request as draft May 31, 2023 22:24
@bdice
Copy link
Contributor

bdice commented Mar 7, 2024

This PR successfully implemented NVTX from GitHub for cudf: rapidsai/cudf#15178

We can probably copy from it and start a new attempt at this for cuSpatial if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team cmake Related to CMake code or build configuration feature request New feature or request libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change Python Related to Python code
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants