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

reformated device_api and host_api benchmark to include engine, distribution, mode, throughput gigabytes per second, lambda columns #536

Merged
merged 10 commits into from
Aug 21, 2024

Conversation

NguyenNhuDi
Copy link
Collaborator

No description provided.

benchmark/benchmark_rocrand_host_api.cpp Outdated Show resolved Hide resolved
benchmark/custom_csv_formater.hpp Outdated Show resolved Hide resolved
benchmark/custom_csv_formater.hpp Outdated Show resolved Hide resolved
benchmark/custom_csv_formater.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@umfranzw umfranzw left a comment

Choose a reason for hiding this comment

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

Hi Di, I think this looks great - just one quick question though.
I noticed that I do not see the new columns show up when I run with:
./benchmark/benchmark_rocrand_device_api --benchmnark_format=csv

Is there anything special I'd need to do to show them?

@NguyenNhuDi
Copy link
Collaborator Author

NguyenNhuDi commented Aug 19, 2024

Hi @umfranzw, you are right, google benchmark distinguishes between --benchmark_format and --benchmark_out_format.
--benchmark_format will change what appears on the terminal, while --benchmark_out_format will change what is outputted to your output file. I am working on changing up --benchmark_format as well and should have a new commit shortly

@NguyenNhuDi NguyenNhuDi requested a review from a team as a code owner August 19, 2024 20:30
@NguyenNhuDi NguyenNhuDi requested a review from umfranzw August 20, 2024 16:17
Copy link
Collaborator

@umfranzw umfranzw left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@NguyenNhuDi
Copy link
Collaborator Author

As per @RobsonRLemos , we have decided to add 3 more columns, Mode, gigabytes per second and Lambda for consistency with legacy benchmarks. @umfranzw could you review these changes?
Thanks!

@NguyenNhuDi NguyenNhuDi changed the title reformated device_api and host_api benchmark to include engine and distribution columns reformated device_api and host_api benchmark to include engine, distribution, mode, gigabytes per second, lambda columns Aug 20, 2024
@NguyenNhuDi NguyenNhuDi changed the title reformated device_api and host_api benchmark to include engine, distribution, mode, gigabytes per second, lambda columns reformated device_api and host_api benchmark to include engine, distribution, mode, throughput gigabytes per second, lambda columns Aug 20, 2024
@NguyenNhuDi NguyenNhuDi requested a review from umfranzw August 21, 2024 19:59
@NguyenNhuDi
Copy link
Collaborator Author

sorry @umfranzw miss clicked

Copy link
Collaborator

@RobsonRLemos RobsonRLemos left a comment

Choose a reason for hiding this comment

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

The columns included in the benchmark output are looking good. Thanks.

std::string disName = std::string(temp.begin(), temp.begin() + temp.find(">"));

Out << engineName << ",";
Out << disName << ",";
std::string lambda = "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably not significant enough to require a change, but just for future reference, I believe C++ strings have a constructor that initializes them to the empty string, so you don't need to set them like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, thank you

size_t ePos = disName.find("=");
if(ePos <= disName.size())
{
lambda = std::string(disName.begin() + (ePos + 1), disName.end() - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the string parsing in this function is fine for this task, because we're expecting things to always be present and always be in the same order. However, just for future reference, using a regular expression with capture groups can be really useful in situations where you need to pull out a bunch of sub-strings like this. You can create a regex pattern that contains all the delimiters ('<', ','), apply it to the original temp string, and then iterate through the captured text in a single loop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay thank you, sounds good

@umfranzw
Copy link
Collaborator

sorry @umfranzw miss clicked

No worries, I took a look at the new changes, and I think everything still looks good.

@NguyenNhuDi NguyenNhuDi merged commit e95eef9 into ROCm:develop Aug 21, 2024
26 checks passed
@NguyenNhuDi NguyenNhuDi deleted the zenguyen/benchmark-reformat branch November 21, 2024 20:27
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.

3 participants