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

Utilize string_view initialization of TfToken in path parsing #3054

Closed

Conversation

nvmkuruc
Copy link
Collaborator

@nvmkuruc nvmkuruc commented Apr 24, 2024

Description of Change(s)

(Depends on #3053)

GetToken tries to minimize dynamic allocations during path parsing by utilizing a stack allocated char[32] that can be explicitly NULL-terminated. This optimization still involves a copy to the intermediate buffer and only works for short strings.

This changes GetToken to use the new std::string_view constructor for TfToken instead. When the components of the path already exist in the registry, no copies or allocations are required. We've seen performance gains of about 4-8% in a simple test case where all the path components already exist as tokens.

Fixes Issue(s)

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@nvmkuruc nvmkuruc force-pushed the optimizepathconstruction branch from 757d7d0 to ff064eb Compare April 24, 2024 19:48
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-9589

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nvmkuruc nvmkuruc force-pushed the optimizepathconstruction branch 2 times, most recently from 80e7273 to fd3c4a3 Compare April 25, 2024 20:48
@nvmkuruc nvmkuruc force-pushed the optimizepathconstruction branch from fd3c4a3 to bfc7ebe Compare May 6, 2024 18:34
@nvmkuruc nvmkuruc force-pushed the optimizepathconstruction branch from bfc7ebe to 9c0d0fb Compare May 31, 2024 19:15
@nvmkuruc nvmkuruc force-pushed the optimizepathconstruction branch from 9c0d0fb to 9a898d3 Compare June 11, 2024 18:47
@nvmkuruc nvmkuruc force-pushed the optimizepathconstruction branch from 9a898d3 to a24b27d Compare August 26, 2024 18:53
@nvmkuruc nvmkuruc force-pushed the optimizepathconstruction branch from a24b27d to 83831a9 Compare September 18, 2024 19:22
@Denise-Yang
Copy link

Denise-Yang commented Dec 9, 2024

Hi @nvmkuruc! Thanks for proposing this change, however we are leaning towards closing this out for now.
While we do appreciate the spirit of this PR, our main concern is that it increases the size of _Rep due the proposed std::string_view member, and our suite of performance tests demonstrated more regressions than gains. Furthermore with C++23, we will not need the const *char and the proposed std::string_view members since we will no longer need to construct temporary strings when inserting. As a result, we are thinking of including this optimization once we move to C++23.

However if you are interested in adding a string_view API or if the performance results for parsing paths are very compelling for you, then we could consider adding a more limited API that just introduces a string_view constructor or consider replacing the const * char member with a type that does not cause _Rep to bloat.

@Denise-Yang
Copy link

Closing this PR out because this change causes _Rep to increase in size, and when we upgrade to C++23, we will be able to address the issue that this PR brings up without needing to bloat _Rep.

@pixar-oss pixar-oss closed this Dec 16, 2024
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.

4 participants