Autodesk: Add new options to enable or disable the trace and the performance log in Hd

[erikaharrison-adsk]

Description of Change(s)

Add options to enable or disable tracing. And add another option to enable or disable the performance log in hd.
Tracing and performance log is valuable for profiling. But in a formal release, they will impact the performance. So we should add the ability to disable the two features.

By default, both features are still enabled.

Link to proposal (if applicable)

• N/A

Fixes Issue(s)

• N/A

Checklist

• I have created this PR based on the dev branch

• I have followed the coding conventions

• I have added unit tests that exercise this functionality (Reference:
  testing guidelines)

• I have verified that all unit tests pass with the proposed changes

• I have submitted a signed Contributor License Agreement (Reference:
  Contributor License Agreement instructions)

[jesschimein]

Filed as internal issue #USD-11029

(This is an automated message. See here for more information.)

[spiffmon]

We are very sympathetic to the desires for this change! We are fine with changing TRACE_DISABLE into TRACE_ENABLE for consistency. But (and this applies to the newly introduced directive for Hd, also) we are not OK with requiring the build system to define the directive in order for the feature to be activated.

So, can you change teh guards from #if defined(TRACE_ENABLE) to #if TRACE_ENABLE and add a block at the top like

#if !defined(TRACE_ENABLE)
    #define TRACE_ENABLE 1
#endif

Thank you!

[PierreWang]

We are very sympathetic to the desires for this change! We are fine with changing TRACE_DISABLE into TRACE_ENABLE for consistency. But (and this applies to the newly introduced directive for Hd, also) we are not OK with requiring the build system to define the directive in order for the feature to be activated.

So, can you change teh guards from #if defined(TRACE_ENABLE) to #if TRACE_ENABLE and add a block at the top like

#if !defined(TRACE_ENABLE)
    #define TRACE_ENABLE 1
#endif

Thank you!

Hi @spiffmon , thank you for your comments. For the HD_PERF_ENABLE, we also need to add the similar block, right? And I think we also need to remove the changes in build system?

[spiffmon]

I believe so, yes.

[erikaharrison-adsk]

Branch updated. Let us know if this addresses your concerns.

[spiffmon]

Thanks, @erikaharrison-adsk - this looks great!

[cvj]

There's still another issue here we need to address.

We would like the build to still succeed if we disable the TRACE and HD_PERF macros, but this isn't possible with the current change. The total removal of the HdPerfLog class prevents several tests from building.

We could work around this if we just made disabling the HD_PERF macros define no-ops, but left the HdPerfLog class defined. This would let tests that depend on HdPerfLog continue to build though they would fail when run (which is fine and to be expected). I have modified the change to do this locally and it does successfully build.

I think this would give the outcome everyone wants (TRACE and HD_PERF calls are not present in the core libraries and builds still work). Does this work for everybody?

[cvj]

There's still another issue here we need to address.

We would like the build to still succeed if we disable the TRACE and HD_PERF macros, but this isn't possible with the current change. The total removal of the HdPerfLog class prevents several tests from building.

We could work around this if we just made disabling the HD_PERF macros define no-ops, but left the HdPerfLog class defined. This would let tests that depend on HdPerfLog continue to build though they would fail when run (which is fine and to be expected). I have modified the change to do this locally and it does successfully build.

I think this would give the outcome everyone wants (TRACE and HD_PERF calls are not present in the core libraries and builds still work). Does this work for everybody?

@erikaharrison-adsk @PierreWang - Did my proposal above seem acceptable to you?

Also, just FYI, I work at Pixar and am the person landing this change in the tree. I realize that may not have been obvious in my previous comment.

[PierreWang]

@cvj

There's still another issue here we need to address.

We would like the build to still succeed if we disable the TRACE and HD_PERF macros, but this isn't possible with the current change. The total removal of the HdPerfLog class prevents several tests from building.

We could work around this if we just made disabling the HD_PERF macros define no-ops, but left the HdPerfLog class defined. This would let tests that depend on HdPerfLog continue to build though they would fail when run (which is fine and to be expected). I have modified the change to do this locally and it does successfully build.

I think this would give the outcome everyone wants (TRACE and HD_PERF calls are not present in the core libraries and builds still work). Does this work for everybody?

This should work for us. I like this idea. And I will make this change.
I am sorry I was a bit busy last month and forgot to answer this.

[cvj]

@PierreWang

This should work for us. I like this idea. And I will make this change. I am sorry I was a bit busy last month and forgot to answer this.

Great! I actually already have this change made locally (making the HD_PERF macros no-ops but leaving HdPerfLog defined) so you don't need to update this PR in this case, since I can just use what I've already done. I just wanted to make sure this approach would work for you and us.