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

perf: refacto & slight performance improvements #126

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

yo-main
Copy link

@yo-main yo-main commented Mar 11, 2021

Hi !

I closed my previous pull request for this (#125) because I messed up my files and I redid everything from scratch (my apologies for this)....

This PR is about a bunch of small modifications that takes advantages of some python idioms and should slightly improve performances.
I tried not to break any logic, and all tests are working, but I might have missed a few things here or there.

Many thanks !

@yo-main yo-main changed the title perf: refacto & improve performance perf: refacto & slight performance improvements Mar 11, 2021
casbin/model/policy.py Outdated Show resolved Hide resolved
@hsluoyz
Copy link
Member

hsluoyz commented Mar 11, 2021

I'm OK with useful refactoring. But I don't like some changes that make the code obscure. Code readability and maintainability are the first thing we consider.

@yo-main
Copy link
Author

yo-main commented Mar 12, 2021

Hi @hsluoyz, would you mind to let me know which part exactly aren't clear enough ? I don't mind to rework them or set it back as it originally was. But I think some part would really bring a performance boost.

We are currently setting up Casbin to manage hundreds of thousands of rules (groups & permissions) and we started to have performance issues when provisioning roles.

Changes like this one or this one would definitively avoid some computation which aren't needed and this would be noticeable for operation done in huge batch.

@hsluoyz
Copy link
Member

hsluoyz commented Mar 12, 2021

@yo-main ok, don't remove the param names like rule_added, or its meaning becomes obscure. I think the two you pointed out look OK.

@hsluoyz hsluoyz reopened this Mar 12, 2021
@yo-main yo-main force-pushed the master branch 4 times, most recently from d5805ce to 070a781 Compare March 12, 2021 13:27
@yo-main
Copy link
Author

yo-main commented Mar 12, 2021

Done, I do agree with you that it makes it less readable. I've reverted my changes. Thank you for the feedback !

Copy link
Contributor

@Zxilly Zxilly left a comment

Choose a reason for hiding this comment

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

lgtm

casbin/core_enforcer.py Outdated Show resolved Hide resolved
test_filter.py Show resolved Hide resolved
Signed-off-by: Romain Arnal <romain.arnal@sewan.fr>
@hsluoyz hsluoyz merged commit ba52d14 into casbin:master Mar 12, 2021
@hsluoyz
Copy link
Member

hsluoyz commented Mar 12, 2021

🎉 This PR is included in version 0.18.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants