Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace Clone() with View() #432

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

HieDean
Copy link
Contributor

@HieDean HieDean commented Nov 17, 2023

Replace Clone() with View()

@csukuangfj
Copy link
Collaborator

There are also other occurrences of Clone() that can be replaced with View. For instance,

ans.push_back(Clone(Allocator(), &attn_cache_));
ans.push_back(Clone(Allocator(), &conv_cache_));

ans.emplace_back(Clone(allocator_, &s));

The returned states are inputs to the neural network and won't be changed by the callers, so sharing the memory with View() should be safe.

@csukuangfj
Copy link
Collaborator

By the way,please fix the code stle issues
https://github.com/k2-fsa/sherpa-onnx/actions/runs/6900224733/job/18773046296?pr=432

You can check the code style locally by running

cd /path/to/sherpa-onnx
./scripts/check_style_cpplint.sh 2

@HieDean HieDean force-pushed the replace_Clone_with_View branch from 3467c5b to c6acdc0 Compare November 18, 2023 13:19
@HieDean
Copy link
Contributor Author

HieDean commented Nov 18, 2023

Follow your guide and make changes.

@HieDean
Copy link
Contributor Author

HieDean commented Nov 18, 2023

For

ans.push_back(Clone(Allocator(), &attn_cache_));
ans.push_back(Clone(Allocator(), &conv_cache_));
and
ans.emplace_back(Clone(allocator_, &s));

I leave them unchaged. I am not sure using View() in GetInitStates() is a good idea, because GetInitStates() is designed to have no right to modify member variables.

@csukuangfj
Copy link
Collaborator

Follow your guide and make changes.

Thanks!

because GetInitStates() is designed to have no right to modify member variables.

Please change

std::vector<Ort::Value> GetInitStates() const {

to

 std::vector<Ort::Value> GetInitStates() { 

by removing the trailing const.

If the CI passes, then it is safe to use View in GetInitStates().

@HieDean HieDean force-pushed the replace_Clone_with_View branch from c6acdc0 to 83d0bc0 Compare November 19, 2023 14:45
@@ -250,7 +250,7 @@ Ort::Value OnlineLstmTransducerModel::RunDecoder(Ort::Value decoder_input) {
Ort::Value OnlineLstmTransducerModel::RunJoiner(Ort::Value encoder_out,
Ort::Value decoder_out) {
std::array<Ort::Value, 2> joiner_input = {std::move(encoder_out),
std::move(decoder_out)};
View(&decoder_out)};
Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, we don't need to replace move with View.

}
return {std::move(Clone(allocator_, &init_scores_.value)), std::move(ans)};
return {std::move(View(&init_scores_.value)), std::move(ans)};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return {std::move(View(&init_scores_.value)), std::move(ans)};
return {View(&init_scores_.value), std::move(ans)};

@HieDean HieDean force-pushed the replace_Clone_with_View branch from 83d0bc0 to 56ad64e Compare November 19, 2023 15:19
@csukuangfj
Copy link
Collaborator

Thank you for your contribution!

@csukuangfj csukuangfj merged commit e6a2d0d into k2-fsa:master Nov 20, 2023
@HieDean
Copy link
Contributor Author

HieDean commented Nov 20, 2023

Thank you for your guidance! I feel very happy that I can contribute to the community. And I have learned a lot during PR.

XiaYucca pushed a commit to XiaYucca/sherpa-onnx that referenced this pull request Jan 9, 2025
Co-authored-by: hiedean <hiedean@tju.edu.cn>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants