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
Conversation
@arthuridea can you see if this PR solves your issue? |
@huazhikui @nodece @xcaptain please review. |
I wrote a simple dotnet core api project just now ,loading 9000 policies.The adapter was injected as Scoped service. It costs 2.05s in Waiting(TFFB) under version 1.2.7,while it successfully reduces to 72ms without any code changes. I didn't make any further testing,maybe this version works!Thanks very much for all of you. |
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 HashSet<string>
may cause some problems.
set => _policy = value; | ||
get => _policy; | ||
Policy = new List<List<string>>(); | ||
PolicyStringSet = new HashSet<string>(); |
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, like
p, 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 :
for (var i = 0; i < a.Count; i++)
{
if (!a[i].Equals(b[i]))
{
return false;
}
}
return true;
If a rule contains the same string and the HasPolicy
will can not return false at before too. Whether the HashSet<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:
class ListPolicy<T> : List<T>
{
public override bool Equals(object obj)
{
// I'm not sure how to write.
return obj is ListPolicy<T> policy &&
Capacity == policy.Capacity &&
Count == policy.Count;
}
public override int GetHashCode()
{
var hash = new HashCode();
foreach (var item in this)
{
hash.Add(item);
}
return hash.ToHashCode();
}
}
class MainClass
{
public static void Main(string[] args)
{
var policy = new HashSet<ListPolicy<string>>
{
new ListPolicy<string> { "user","data1","read" },
new ListPolicy<string> { "user","data1","read" }
};
Console.WriteLine(policy.Count); // => 1
}
}
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:class ListPolicy<T> : List<T> { public override bool Equals(object obj) { // I'm not sure how to write. return obj is ListPolicy<T> policy && Capacity == policy.Capacity && Count == policy.Count; } public override int GetHashCode() { var hash = new HashCode(); foreach (var item in this) { hash.Add(item); } return hash.ToHashCode(); } } class MainClass { public static void Main(string[] args) { var policy = new HashSet<ListPolicy<string>> { new ListPolicy<string> { "user","data1","read" }, new ListPolicy<string> { "user","data1","read" } }; Console.WriteLine(policy.Count); // => 1 } }
I mentioned it in the next review. here is the content:
If changed List<List<string>>
to Hash<Rule>
(the Rule
inherit List<string>
and overwrite the Equals
and GetHashCode
), It will change almost all the public API and can not gain more performance. It will cost much time when Add
the list because the Equal
or Add
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 :
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19645
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100-preview.5.20279.10
[Host] : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
Job-WRAGMF : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
IterationCount=3 RunStrategy=Throughput
Method | Times | PolicyCount | Mean | Error | StdDev | Min | Max | Median |
---|---|---|---|---|---|---|---|---|
HasPolicy | 1000 | 500 | 511.904 μs | 331.9158 μs | 18.1934 μs | 492.169 μs | 528.009 μs | 515.534 μs |
AddPolicy | 1000 | 500 | 10,286.494 μs | 1,392.4007 μs | 76.3222 μs | 10,239.692 μs | 10,374.566 μs | 10,245.225 μs |
HasPolicy | 1000 | 5000 | 523.930 μs | 411.9189 μs | 22.5787 μs | 498.292 μs | 540.851 μs | 532.646 μs |
AddPolicy | 1000 | 5000 | 10,284.017 μs | 5,746.3454 μs | 314.9766 μs | 9,943.827 μs | 10,565.527 μs | 10,342.697 μs |
return false; | ||
|
||
Assertion assertion = Model[sec][ptype]; | ||
assertion.Policy.Add(rule); |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
If changed List<List<string>>
to Hash<Rule>
(the Rule
inherit List<string>
and overwrite the Equals
and GetHashCode
), It will change almost all the public API and can not gain more performance. It will cost much time when Add
the list because the Equal
or Add
has O(n) complexity.
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.
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 comment
The 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.
We can try to make it better, I think we can convert it to an array when using 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.
the assertion.PolicyStringSet
proptry does not need to be public.
- add
assertion.AddPolicy(rule)
method inAssertion Class
replace these 2 steps:
assertion.Policy.Add(rule);
assertion.PolicyStringSet.Add(Utility.ArrayToString(rule));
-
add
assertion.Contains(rule)
mehod inAssertion Class
replaceModel[sec][ptype].PolicyStringSet.Contains(Utility.ArrayToString(rule))
-
add
assertion.RemovePolicyAt(i)
method replace these 2 steps:
assertion.Policy.RemoveAt(i);
assertion.PolicyStringSet.Remove(Utility.ArrayToString(rule));
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, The assertion.PolicyStringSet
is Internal now, I will add them at next commit.
close #15
I created a benchmark project and here is a partial result.
Before
Now