-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Sort during find results #947
Conversation
Codecov Report
@@ Coverage Diff @@
## master #947 +/- ##
==========================================
+ Coverage 45.39% 45.72% +0.33%
==========================================
Files 178 178
Lines 12662 12732 +70
==========================================
+ Hits 5748 5822 +74
+ Misses 6914 6910 -4
Continue to review full report at Codecov.
|
neo/IO/Caching/DataCache.cs
Outdated
{ | ||
lock (dictionary) | ||
{ | ||
return FindUnsorted(key_prefix).OrderBy(u => u.Key, 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.
This forces the entire set of search results to be loaded into memory for sorting, which can lead to attacks.
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.
Still nowadays I have dificulties with C#.
Looking at it I would think that it finds and returns the set, then, it sorts the results.
In this sense, maybe it needs to be split into two steps, right, @erikzhang?
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.
We need to do some optimization. Because the records read from the underlying storage (leveldb) is already sorted.
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.
Perfect, master.
We are going to reflect on that and think about an efficient way.
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 will propose another way
|
||
// We need to return a, b and cached results between them | ||
|
||
if (a.Key == null && a.Value == null) |
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.
ReferenceEquals(a,n) doesn't work :S
Close #946