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

fix: expedite benchmark tests #159

Closed
wants to merge 2 commits into from
Closed

fix: expedite benchmark tests #159

wants to merge 2 commits into from

Conversation

noob20000405
Copy link
Contributor

@noob20000405 noob20000405 commented Nov 1, 2021

Fix: #155

Description

There were some loops for benchmark tests used in some functions that could take long time, e.g.,

BenchmarkRBACModelSmall() 
BenchmarkRBACModelMedium()
BenchmarkRBACModelLarge()

Except the number of iterations of loop, these functions were almost the same.
I deleted those functions called *Medium() and *Large(), then replaced the name of *Small() by *Loop(), it can save a lot of time.

Screenshots/Testimonials

image

@casbin-bot
Copy link
Member

@EmperorYP7 @divy9881 please review

Signed-off-by: noob20000405 <vincent4869code@gmail.com>
@hsluoyz
Copy link
Member

hsluoyz commented Nov 1, 2021

@sheny1xuan plz review

Copy link
Member

@EmperorYP7 EmperorYP7 left a comment

Choose a reason for hiding this comment

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

@noob20000405 Kindly make sure you checkout to another branch and create a PR from it to casbin:master from now on. You might need to fork this repo several times or make many merge commits on noob20000405:master branch if you don't do this.

@noob20000405
Copy link
Contributor Author

checkout to another branch

Thank you sincerely for your help, so for this time am I supposed to recreate a PR?

@EmperorYP7
Copy link
Member

Thank you sincerely for your help, so for this time am I supposed to recreate a PR?

No. This PR will suffice for now.

@@ -60,53 +60,7 @@ static void BenchmarkCachedRBACModelSmall(benchmark::State& state) {
e.Enforce(params);
}

BENCHMARK(BenchmarkCachedRBACModelSmall);
Copy link
Member

Choose a reason for hiding this comment

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

I think commenting this out is better than deleting it right away. If we comment out only this specific line, the benchmark for BenchmarkCachedRBACModelSmall won't run. However, the user has the option to run it anyway.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about this @noob20000405?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EmperorYP7 Totally agree with you, I'll change it now.

e.Enforce(params);
}

// BENCHMARK(BenchmarkCachedRBACModelMedium);
Copy link
Member

Choose a reason for hiding this comment

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

Likewise

params = { "user" + std::to_string(GetRandom1000()), "data" + std::to_string(GetRandom1000()/10), "read" }, e.HasPolicy(params);
}

BENCHMARK(BenchmarkHasPolicyMedium);
Copy link
Member

Choose a reason for hiding this comment

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

Likewise

}
}

BENCHMARK(BenchmarkHasPolicyLarge);
Copy link
Member

Choose a reason for hiding this comment

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

Likewise

@@ -101,38 +73,9 @@ static void BenchmarkAddPolicySmall(benchmark::State& state) {
params = {"user" + std::to_string(GetRandom100() + 100), "data" + std::to_string((GetRandom100() + 100)/10), "read"}, e.AddPolicy(params);
}

BENCHMARK(BenchmarkAddPolicySmall);
Copy link
Member

Choose a reason for hiding this comment

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

Here as well

}
}

BENCHMARK(BenchmarkAddPolicyMedium);
Copy link
Member

Choose a reason for hiding this comment

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

and so on. Hope you got the pattern

@hsluoyz
Copy link
Member

hsluoyz commented Nov 2, 2021

Replaced by: #161

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.

Don't run benchmark tests in CI to avoid error
4 participants