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

chore: Pycasbin benchmark #199

Closed
wants to merge 2 commits into from

Conversation

sheny1xuan
Copy link
Contributor

@sheny1xuan sheny1xuan commented Apr 10, 2022

pybind11 heavily relies on a template matching mechanism to convert parameters and return values that are constructed from STL data types such as vectors, linked lists, hash tables, etc. This even works in a recursive manner, for instance to deal with lists of hash maps of pairs of elementary and custom types, etc.
However, a fundamental limitation of this approach is that internal conversions between Python and C++ types involve a copy operation that prevents pass-by-reference semantics.

@casbin-bot
Copy link
Member

@EmperorYP7 @divy9881 @noob20000405 please review

@hsluoyz
Copy link
Member

hsluoyz commented Apr 10, 2022

@EmperorYP7 plz review

@@ -82,6 +82,12 @@ def isAuthorized(req):

Rest of the method's name is on par with `casbin-cpp`.

### Benchmark

Pycasbin use `pytest` for benchmark.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Pycasbin use `pytest` for benchmark.
Pycasbin use `pytest` for benchmark.
`pip3 install -U pytest`


Pycasbin use `pytest` for benchmark.

Install `pytest` and `pycasbin` in your local machine, then run the benchmark by `python3 -m pytest --benchmark-verbose --benchmark-columns=mean,stddev,iqr,ops,rounds casbin-cpp/pycasbin/benchmarks/benchmark_model.py`.
Copy link
Member

Choose a reason for hiding this comment

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

Do this instead:

python3 -m pytest --benchmark-verbose --benchmark-columns=mean,stddev,iqr,ops,rounds casbin-cpp/pycasbin/benchmarks/benchmark_model.py

e.enforce("user501", "data9", "read")


# TODO: pycasbin cost too much time in large model
Copy link
Member

Choose a reason for hiding this comment

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

Does it crash often?

How about we set up a flag through an OS env variable for intensive testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It cost too much time in my machine, don't crash.

@EmperorYP7
Copy link
Member

Current pybind11 binding copy python list to a vector, it's will be inefficient. And I think wrap DataVector and DataList with python will be helpful.

Flagging these data types as opaque might be one way to go.

And another thing, current casbin-cpp efficiency is smiliar with casbin-golang. Can we make it better? I think I need some help for the deep optimization.

Let's perform profiling on the current casbin-cpp and pycasbin setup. I suspect casbin-cpp's performance majorly depends on the performance of the underlying library used. (Exprtk)

There are scopes for micro-optimizations that I can think of. We'd need a thorough look into the code to create a significant difference in performance.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 12, 2022

@sheny1xuan

@ArashPartow
Copy link
Contributor

@sheny1xuan in the first set of timing numbers, the stddev is greater than the mean.

This usually indicates invalid stats or the possibility of at least a bimodal (or more) distribution.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 14, 2022

@sheny1xuan

@sheny1xuan
Copy link
Contributor Author

sheny1xuan commented Apr 14, 2022

@sheny1xuan in the first set of timing numbers, the stddev is greater than the mean.

This usually indicates invalid stats or the possibility of at least a bimodal (or more) distribution.

Oh, I only paid attention to the mean, the stddev is also very important in the benchmark. And the above comparison of pycasbin and casbin-cpp binding is valid. The two set of benchmark data is all about casbin-cpp binding, I forget to change the package install environment.
And I rerun it in my local machine, the stddev is still greater than mean. Stddev is greater means the test data is unstable. I think there maybe something wrong in the code about binding or benmarking.
image

@sheny1xuan sheny1xuan closed this Apr 26, 2022
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.

5 participants