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

Replace NVTX macros with constexpr + namespaced methods #990

Merged
merged 1 commit into from
Nov 24, 2022

Conversation

ptheywood
Copy link
Member

@ptheywood ptheywood commented Nov 21, 2022

  • Replaces NVTX_PUSH(label) with flamegpu::util::nvtx::push(label)
  • Replaces NVTX_POP() with flamegpu::util::nvtx::pop()
  • Replaces NVTX_RANGE(label) with flamegpu::nvtx::Range r(label)
  • Adds cosntexpr bool flamegpu::nvtx::ENABLED
  • Wraps flamegpu::util::nvtx with swig, exluding range
    • pyflamegpu.NVTX_ENABLED, pyflamegpu.nvtx_push(label), pyflamegpu.nvtx_pop()

Should have the same (C++) runtime cost in optimisaed builds

Closes #984

@ptheywood
Copy link
Member Author

ptheywood commented Nov 21, 2022

Ranges can be generated from python now:

nsys profile -o test python3 -m pytest ../tests/swig/python/util/test_nvtx.py 

image

Might need some tweaking if / when we have a static installable build of the c++ library, as the ENABLED property is just fed from the macro.

Not checked this produces the same binaries, that's a much bigger effort to diff than a standalone incomplete MWE.

@ptheywood ptheywood marked this pull request as ready for review November 22, 2022 14:14
Copy link
Member

@mondus mondus left a comment

Choose a reason for hiding this comment

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

Suggest to disable Range in Python to avoid potential Python garbage collection problems.

@ptheywood
Copy link
Member Author

Now re-adjusted to:

  • uses a namespace again, rathe rthan class
  • lowercase range
  • does not wrap range, python is now nvtx_push via rename (due to use of namespace not class)
  • fixed a lifetime bug in C++ (need to assign to a variable for scoping to be useful, another minor downside compared to the macro).

image

Needs a squash, but subject to windows specific warnings about unused variables / masking this should be good to re-review.

+ Replaces NVTX_PUSH(label) with flamegpu::util::nvtx::push(label)
+ Replaces NVTX_POP() with flamegpu::util::nvtx::pop()
+ Replaces NVTX_RANGE(label) with flamegpu::util::nvtx::Range(label)
  + Ranges must be assigned to a variable for correct lifetime
+ Add constexpr bool flamegpu::util::nvtx::ENABLED
+ Wraps flamegpu::util::nvtx with swig, excluding ranges due to Swig + python GC concerns
  + Adds pyflamegpu.nvtx_push(lable)
  + Adds pyflamegpu.nvtx_pop()
  + Adds pyflamegpu.NVTX_ENABLED
  + Adds python tests
Copy link
Member

@Robadob Robadob left a comment

Choose a reason for hiding this comment

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

Changes look good, will merge after CI is happy.

@Robadob Robadob merged commit 32addb1 into master Nov 24, 2022
@Robadob Robadob deleted the constexpr-nvtx branch November 24, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use if constexpr for namespaced NVTX_PUSH etc
3 participants