[onnx.export] Avoid unnecessary copy of debug_names #123026
Conversation
gustavla
requested review from
BowenBao,
abock and
thiagocrepaldi
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/123026
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 0f522ce with merge base a91311e ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
the
release notes: onnx
thiagocrepaldi
added
module: onnx
janeyx99
added
the
triaged
|
@justinchuby Let me know if there is any changes needed to this one, thanks! |
torch/csrc/jit/passes/onnx.cpp
Outdated
| // Note: calls to setDebugName will change debug_names and corrupt | ||
| // exist_name |
There was a problem hiding this comment.
The original comments seem to be clearer, (even though I don't understand the behavior completely). Do you have an idea of what happens here; could you potentially improve the note?
There was a problem hiding this comment.
This is really subtle, so I have taken a stab at making this even more clear. I'll let the code change speak for itself, so let me know if it's clear now.
|
@justinchuby Good to merge? (I can't issue that even with approval) Thanks! |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
These failures look unrelated. I may need to rebase and push again. That seems to do the trick in these situations. |
- This ones requires us to be careful, since calls to `setDebugName` changes `debug_names` and invalidates the `exist_name` iterator. So if we simply change `auto` to `const auto&`, then between that line and `find` we have corrupted the iterator by calling `output[i]->setDebugName`. This change aims to be functionally equivalent to the original, which is why we first get the Value pointer, then call `output[i]->setDebugName`, and finally call `setDebugName` on the found value. It is possible functionally it is OK to simply call `output[i]->setDebugName` first and then find and the second `setDebugName`, but this would not be identical to current behavior. - Resolves (2) in pytorch#121422.
gustavla
force-pushed
the
dev/gustav/onnx_export_speedup_121422_B_1
branch
from
d19dd69
to
0f522ce
Compare
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
This PR is part of an effort to speed up torch.onnx.export (pytorch#121422). - The `auto debug_names = ` infers a copy, where as `const auto& debug_names` does not. - However, this ones requires us to be careful, since calls to `setDebugName` changes `debug_names` and invalidates the `exist_name` iterator. So if we simply change `auto` to `const auto&`, then between that line and `find` we have corrupted the iterator by calling `output[i]->setDebugName`. This change aims to be functionally equivalent to the original, which is why we first get the Value pointer, then call `output[i]->setDebugName`, and finally call `setDebugName` on the found value. It is possible functionally it is OK to simply call `output[i]->setDebugName` first and then find and the second `setDebugName`, but this would not be identical to current behavior. - Resolves (2) in pytorch#121422. Pull Request resolved: pytorch#123026 Approved by: https://github.com/justinchuby
This PR is part of an effort to speed up torch.onnx.export (#121422).
auto debug_names =infers a copy, where asconst auto& debug_namesdoes not.setDebugNamechangesdebug_namesand invalidates theexist_nameiterator. So if we simply changeautotoconst auto&, then between that line andfindwe have corrupted the iterator by callingoutput[i]->setDebugName. This change aims to be functionally equivalent to the original, which is why we first get the Value pointer, then calloutput[i]->setDebugName, and finally callsetDebugNameon the found value. It is possible functionally it is OK to simply calloutput[i]->setDebugNamefirst and then find and the secondsetDebugName, but this would not be identical to current behavior.