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
Improve performance when using large amount policy #39
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
using NetCasbin.Rbac; | ||
using NetCasbin.Util; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
|
@@ -14,28 +13,39 @@ public void BuildRoleLinks(IRoleManager rm) | |
{ | ||
if (Model.ContainsKey("g")) | ||
{ | ||
foreach (var ast in Model["g"].Values) | ||
foreach (Assertion ast in Model["g"].Values) | ||
{ | ||
ast.BuildRoleLinks(rm); | ||
} | ||
} | ||
} | ||
|
||
public void RefreshPolicyStringSet() | ||
{ | ||
foreach (Assertion ast in Model.Values | ||
.SelectMany(pair => pair.Values)) | ||
{ | ||
ast.RefreshPolicyStringSet(); | ||
} | ||
} | ||
|
||
public void ClearPolicy() | ||
{ | ||
if (Model.ContainsKey("p")) | ||
{ | ||
foreach (var ast in Model["p"].Values) | ||
foreach (Assertion ast in Model["p"].Values) | ||
{ | ||
ast.Policy = new List<List<string>>(); | ||
ast.RefreshPolicyStringSet(); | ||
} | ||
} | ||
|
||
if (Model.ContainsKey("g")) | ||
{ | ||
foreach (var ast in Model["p"].Values) | ||
foreach (Assertion ast in Model["p"].Values) | ||
{ | ||
ast.Policy = new List<List<string>>(); | ||
ast.RefreshPolicyStringSet(); | ||
} | ||
} | ||
} | ||
|
@@ -73,29 +83,37 @@ public List<List<string>> GetFilteredPolicy(string sec, string ptype, int fieldI | |
|
||
public bool HasPolicy(string sec, string ptype, List<string> rule) | ||
{ | ||
return Model[sec][ptype]!=null && Model[sec][ptype].Policy.Any(x => Utility.ArrayEquals(rule, x)); | ||
return Model[sec][ptype] != null && Model[sec][ptype].PolicyStringSet.Contains(Utility.ArrayToString(rule)); | ||
} | ||
|
||
public bool AddPolicy(string sec, string ptype, List<string> rule) | ||
{ | ||
if (!HasPolicy(sec, ptype, rule)) | ||
if (HasPolicy(sec, ptype, rule)) | ||
{ | ||
Model[sec][ptype].Policy.Add(rule); | ||
return true; | ||
return false; | ||
} | ||
return false; | ||
|
||
Assertion assertion = Model[sec][ptype]; | ||
assertion.Policy.Add(rule); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we merge these 2 steps into 1? We can compare 2 List, why change to string There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have a lot of policies in memory, will this increase memory usage because of storing twice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, But it will not much increase memory and to file adapter it can be like a cache. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the
assertion.Policy.Add(rule);
assertion.PolicyStringSet.Add(Utility.ArrayToString(rule));
assertion.Policy.RemoveAt(i);
assertion.PolicyStringSet.Remove(Utility.ArrayToString(rule)); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, The |
||
assertion.PolicyStringSet.Add(Utility.ArrayToString(rule)); | ||
return true; | ||
} | ||
|
||
public bool RemovePolicy(string sec, string ptype, List<string> rule) | ||
{ | ||
for (var i = 0; i < Model[sec][ptype].Policy.Count; i++) | ||
if (!HasPolicy(sec, ptype, rule)) | ||
{ | ||
var r = Model[sec][ptype].Policy[i]; | ||
if (Utility.ArrayEquals(rule, r)) | ||
{ | ||
Model[sec][ptype].Policy.RemoveAt(i); | ||
return true; | ||
} | ||
return true; | ||
} | ||
|
||
Assertion assertion = Model[sec][ptype]; | ||
for (var i = 0; i < assertion.Policy.Count; i++) | ||
{ | ||
var r = assertion.Policy[i]; | ||
if (!Utility.ArrayEquals(rule, r)) continue; | ||
assertion.Policy.RemoveAt(i); | ||
assertion.PolicyStringSet.Remove(Utility.ArrayToString(rule)); | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
@@ -105,7 +123,8 @@ public bool RemoveFilteredPolicy(string sec, string ptype, int fieldIndex, param | |
var tmp = new List<List<string>>(); | ||
var res = false; | ||
|
||
foreach (var rule in Model[sec][ptype].Policy) | ||
Assertion assertion = Model[sec][ptype]; | ||
foreach (var rule in assertion.Policy) | ||
{ | ||
var matched = true; | ||
for (var i = 0; i < fieldValues.Length; i++) | ||
|
@@ -128,18 +147,16 @@ public bool RemoveFilteredPolicy(string sec, string ptype, int fieldIndex, param | |
} | ||
} | ||
|
||
Model[sec][ptype].Policy = tmp; | ||
assertion.Policy = tmp; | ||
assertion.RefreshPolicyStringSet(); | ||
return res; | ||
} | ||
|
||
public List<string> GetValuesForFieldInPolicy(string sec, string ptype, int fieldIndex) | ||
{ | ||
var values = new List<string>(); | ||
|
||
foreach (var rule in Model[sec][ptype].Policy) | ||
{ | ||
values.Add(rule[fieldIndex]); | ||
} | ||
var values = Model[sec][ptype].Policy | ||
.Select(rule => rule[fieldIndex]) | ||
.ToList(); | ||
|
||
Utility.ArrayRemoveDuplicates(values); | ||
return values; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
PolicyStringSet
just used to store rule? what if a rule contains same string, likep, alice, alice, read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It stores the string which connected by rule
List<string>
, I think it will keep the same operation as before.this is operation before :
If a rule contains the same string and the
HasPolicy
will can not return false at before too. Whether theHashSet<string>
is safe enough need the rule parsing is complete.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, understand, you turn an array into a string, so no need to worry about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we don't use
HashSet<List<string>
? somethings like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned it in the next review. here is the content:
If changed
List<List<string>>
toHash<Rule>
(theRule
inheritList<string>
and overwrite theEquals
andGetHashCode
), It will change almost all the public API and can not gain more performance. It will cost much time whenAdd
the list because theEqual
orAdd
has O(n) complexity.Additionally, The
HashSet
can not get by index and will shuffle the list (But we can add the index property to handle it).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nodece @xcaptain Here is the benchmark result of using
HashSet<Rule>
, the code is here change-to-hashset branch :