NVIDIA / NVTX

The NVIDIA® Tools Extension SDK (NVTX) is a C-based Application Programming Interface (API) for annotating events, code ranges, and resources in your applications.
Apache License 2.0
294 stars 47 forks source link

Deleting the default constructor of some classes #27

Open AntoineFroger opened 3 years ago

AntoineFroger commented 3 years ago

For the following classes, it might make sense to delete the default-constructor:

What is the rational for allowing the creation of an empty event attribute or a thread range with an empty event attribute? I understand that the NVTX API does not prohibit this behavior but a range or event without any attribute will be meaningless for most, if not all, tools.

jrhemstad commented 3 years ago

For event_attributes, I believe the variadic constructors would fail if the default constructor were deleted.

E.g.,

  template <typename... Args>
  NVTX3_RELAXED_CONSTEXPR explicit event_attributes(category const& c, Args const&... args) noexcept
    : event_attributes(args...)
  {
    attributes_.category = c.get_id();
  }

When sizeof...(args) == 0, it will call the default constructor. If that were deleted, this constructor would fail. The default ctor is what ensures any unspecified field of the event attributes is default initialized.

For thread_range/process_range, I don't have strong feelings either way about deleting the default ctors. In Nsight Systems, would these just show up as an unnamed range in the NVTX swimlane? I agree thats of limited usefulness, but it's not really harmful either.

AntoineFroger commented 3 years ago

When sizeof...(args) == 0, it will call the default constructor. If that were deleted, this constructor would fail. The default ctor is what ensures any unspecified field of the event attributes is default initialized.

I see, thanks for the explanation. If we want to discourage it, we could make the default constructor private. But that wouldn't actually prevent people from constructing an empty event_attributes object so it might not make that much sense.

For thread_range/process_range, I don't have strong feelings either way about deleting the default ctors. In Nsight Systems, would these just show up as an unnamed range in the NVTX swimlane? I agree thats of limited usefulness, but it's not really harmful either.

Nsight Systems just shows an unnamed range which has no attribute:

nsys

I think the most important question is what semantics do we want to give to a default-constructed range. Should it be an unnamed range? Or should it be an empty object that doesn't generate a range?

This intersects the discussion we have in #28. If we want to support "conditional" range (i.e. emit a range "if"), then we might need to change the semantic of the range default-constructor. Note that people that actually want to emit a range with empty attributes could still do it by explicitly creating an empty event_attributes object and passing it to the range constructor.

jrhemstad commented 3 years ago

I think the most important question is what semantics do we want to give to a default-constructed range. Should it be an unnamed range? Or should it be an empty object that doesn't generate a range?

It's a good question. When I'm faced with questions like this, I usually look for inspiration in the STL.

domain_thread_range is most similar to std::lock_guard. lock_guard doesn't allow default construction.

domain_process_range is similar to std::unique_lock whose default ctor exists, but constructs with no associated mutex.

So perhaps that means domain_thread_range's default ctor should be deleted, and domain_process_ranges default ctor should exist, but not start a range.

In re: https://github.com/NVIDIA/NVTX/issues/28, we can accomplish constructing a domain_thread_range object that doesn't start a range with the tag type I described there.