-
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
[Improve
]: scan operations for IReadOnlyStoreView
and fix of Find
with all 0xff keyPrefix
#3688
Conversation
Improve
]: scan operations for IReadOnlyStoreView
Improve
]: scan operations for IReadOnlyStoreView
and fix of Find
with all 0xff keyPrefix
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.
What is the purpose of the cache? What problems is classes
trying to solved or has solved? I am to believe that the cache is suppose to be a faster lookup than from disk. If this is the case, I will fix all cache
classes. These classes are really slow, compared to using leveldb
, rocksdb
or any other key/value
store.
The performance is not the main purpose.
|
Which classes are readlly slow ? |
Co-authored-by: Christopher Schuchardt <[email protected]>
Co-authored-by: Christopher Schuchardt <[email protected]>
Co-authored-by: Christopher Schuchardt <[email protected]>
Co-authored-by: Shargon <[email protected]>
@nan01ab We still want #3688 (comment) updated. @shargon hold until updated. |
You can submit your suggestion. |
How is not necessary. The text is misleading. The exception is misleading. Tell why this is such a big deal to you? What the text is telling people is, that |
Co-authored-by: Christopher Schuchardt <[email protected]>
Great suggestion. |
Changes requests have been resolved.
@@ -112,7 +112,7 @@ public void TestFindEmptyPrefix() | |||
action.Should().Throw<ArgumentException>(); | |||
|
|||
action = () => dataCache.Find([0xff], SeekDirection.Backward).ToArray(); | |||
action.Should().Throw<ArgumentException>(); | |||
action.Should().Throw<NotSupportedException>(); |
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.
Using [0xff]
should work. The way leveldb
and rocksdb
all work is, it seeks
to the key 0xff
and moves backwards from there.
so results would be in this order:
1. 0xff
2. 0xfe
3. 0xfd
4. 0xfc
5. 0xfb
6. 0xfa
7. ...etc...
All you have to do is use ByteArrayComparer
class. I dont understand what the problem is or why no one can change this. its only 2 lines of code.
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.
: ByteArrayComparer.Reverse; | ||
foreach (var (key, value) in view.Seek(inclusiveStartKey, direction)) | ||
{ | ||
if (comparer.Compare(key.ToArray(), exclusiveEndKey) < 0) |
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 want the the key. If the prefix is a key?
if (comparer.Compare(key.ToArray(), exclusiveEndKey) < 0) | |
if (comparer.Compare(key.ToArray(), exclusiveEndKey) <= 0) |
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 want the the key. If the prefix is a key?
exclusiveEndKey
because of exclusive.
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 go to all the trouble to edit or change this. If your not going to fix the issues with the class?
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 go to all the trouble to edit or change this. If your not going to fix the issues with the class?
Because this is ScanRange
/FindRange
, and range is [StartKey, EndKey)
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 want the the key. If the prefix is a key?
start key and end key are not prefix.
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.
Where are you getting this information? What is not to understand?
IStore
doesn't work this way.Find
has bugs inDataCache
.
https://github.com/neo-project/neo/blob/master/src/Neo/Persistence/DataCache.cs#L264
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.
Like I said, DataCache
has bugs.
LevelDBStore
neo/src/Plugins/LevelDBStore/IO/Data/LevelDB/Helper.cs
Lines 21 to 43 in 84a9fd5
public static IEnumerable<(byte[], byte[])> Seek(this DB db, ReadOptions options, byte[]? keyOrPrefix, SeekDirection direction) | |
{ | |
keyOrPrefix ??= []; | |
using var it = db.CreateIterator(options); | |
if (direction == SeekDirection.Forward) | |
{ | |
for (it.Seek(keyOrPrefix); it.Valid(); it.Next()) | |
yield return new(it.Key()!, it.Value()!); | |
} | |
else | |
{ | |
// SeekForPrev | |
it.Seek(keyOrPrefix); | |
if (!it.Valid()) | |
it.SeekToLast(); | |
else if (it.Key().AsSpan().SequenceCompareTo(keyOrPrefix) > 0) | |
it.Prev(); | |
for (; it.Valid(); it.Prev()) | |
yield return new(it.Key()!, it.Value()!); | |
} | |
} |
RocksDbStore
neo/src/Plugins/RocksDBStore/Plugins/Storage/Store.cs
Lines 41 to 52 in 84a9fd5
public IEnumerable<(byte[] Key, byte[] Value)> Seek(byte[]? keyOrPrefix, SeekDirection direction = SeekDirection.Forward) | |
{ | |
keyOrPrefix ??= []; | |
using var it = _db.NewIterator(); | |
if (direction == SeekDirection.Forward) | |
for (it.Seek(keyOrPrefix); it.Valid(); it.Next()) | |
yield return (it.Key(), it.Value()); | |
else | |
for (it.SeekForPrev(keyOrPrefix); it.Valid(); it.Prev()) | |
yield return (it.Key(), it.Value()); | |
} |
MemoryStore
neo/src/Neo/Persistence/MemoryStore.cs
Lines 51 to 63 in 84a9fd5
public IEnumerable<(byte[] Key, byte[] Value)> Seek(byte[]? keyOrPrefix, SeekDirection direction = SeekDirection.Forward) | |
{ | |
keyOrPrefix ??= []; | |
if (direction == SeekDirection.Backward && keyOrPrefix.Length == 0) yield break; | |
var comparer = direction == SeekDirection.Forward ? ByteArrayComparer.Default : ByteArrayComparer.Reverse; | |
IEnumerable<KeyValuePair<byte[], byte[]>> records = _innerData; | |
if (keyOrPrefix.Length > 0) | |
records = records.Where(p => comparer.Compare(p.Key, keyOrPrefix) >= 0); | |
records = records.OrderBy(p => p.Key, comparer); | |
foreach (var pair in records) | |
yield return (pair.Key[..], pair.Value[..]); | |
} |
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.
Like I said,
DataCache
has bugs.LevelDBStore
neo/src/Plugins/LevelDBStore/IO/Data/LevelDB/Helper.cs
Lines 21 to 43 in 84a9fd5
public static IEnumerable<(byte[], byte[])> Seek(this DB db, ReadOptions options, byte[]? keyOrPrefix, SeekDirection direction) { keyOrPrefix ??= []; using var it = db.CreateIterator(options); if (direction == SeekDirection.Forward) { for (it.Seek(keyOrPrefix); it.Valid(); it.Next()) yield return new(it.Key()!, it.Value()!); } else { // SeekForPrev it.Seek(keyOrPrefix); if (!it.Valid()) it.SeekToLast(); else if (it.Key().AsSpan().SequenceCompareTo(keyOrPrefix) > 0) it.Prev(); for (; it.Valid(); it.Prev()) yield return new(it.Key()!, it.Value()!); } } RocksDbStore
neo/src/Plugins/RocksDBStore/Plugins/Storage/Store.cs
Lines 41 to 52 in 84a9fd5
public IEnumerable<(byte[] Key, byte[] Value)> Seek(byte[]? keyOrPrefix, SeekDirection direction = SeekDirection.Forward) { keyOrPrefix ??= []; using var it = _db.NewIterator(); if (direction == SeekDirection.Forward) for (it.Seek(keyOrPrefix); it.Valid(); it.Next()) yield return (it.Key(), it.Value()); else for (it.SeekForPrev(keyOrPrefix); it.Valid(); it.Prev()) yield return (it.Key(), it.Value()); } MemoryStore
neo/src/Neo/Persistence/MemoryStore.cs
Lines 51 to 63 in 84a9fd5
public IEnumerable<(byte[] Key, byte[] Value)> Seek(byte[]? keyOrPrefix, SeekDirection direction = SeekDirection.Forward) { keyOrPrefix ??= []; if (direction == SeekDirection.Backward && keyOrPrefix.Length == 0) yield break; var comparer = direction == SeekDirection.Forward ? ByteArrayComparer.Default : ByteArrayComparer.Reverse; IEnumerable<KeyValuePair<byte[], byte[]>> records = _innerData; if (keyOrPrefix.Length > 0) records = records.Where(p => comparer.Compare(p.Key, keyOrPrefix) >= 0); records = records.OrderBy(p => p.Key, comparer); foreach (var pair in records) yield return (pair.Key[..], pair.Value[..]); }
If DataCache
has bugs, you can submit a PR to fix 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.
This is not **Find**
.
ScanRange
/ FindRange
return items within range [StartKey(inclusive), EndKey(exclusive) )
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.
prefix is misleading word.
it doesnt matter. In the stores the key are in order. If you convert all keys to an
integer
they start fromzero through Infinity
(or biggest key in the store). so if last key is[0xff, 0xff]
and i use prefix0xff
the seeker goto0xff
key.Example with prefix
0xff
for keys ranging from[0x01]
to[0xff, 0xff]
Records returned in order:
Key: [0xff] Key: [0xfe] Key: [0xfd] Key: [0xfc] Key: [0xfb] Key: [0xfa] .... etc ... Key: [0xdf] Key: [0xde] Key: [0xdd] Key: [0xdc] Key: [0xdb] Key: [0xda] .... etc ... Key: [0x01]
You can read this:
neo/src/Neo/Persistence/DataCache.cs
Lines 251 to 258 in 84a9fd5
/// <summary> | |
/// Finds the entries that between [start, end). | |
/// </summary> | |
/// <param name="start">The start key (inclusive).</param> | |
/// <param name="end">The end key (exclusive).</param> | |
/// <param name="direction">The search direction.</param> | |
/// <returns>The entries found with the desired range.</returns> | |
public IEnumerable<(StorageKey Key, StorageItem Value)> FindRange(byte[] start, byte[] end, SeekDirection direction = SeekDirection.Forward) |
/// </exception> | ||
internal static byte[] GetSeekPrefix(this byte[]? keyPrefix, ushort maxSizeWhenAll0xff = 4096 /* make it long enough */) | ||
{ | ||
if (keyPrefix == null) // Backwards seek for null prefix is not supported for now. |
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 is inconsistent with MemoryStore
, LevelDbStore
and RocksDbStore
. Why?
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 tried to fix it. #3680
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.
Fixed in #3701, but that PR isn't going to get merge. So, if you want you can create new PR and use my work.
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.
Fixed in #3701, but that PR isn't going to get merge. So, if you want you can create new PR and use my work.
Yes, I tried to fix it.
But a requirement I received is to maintain consistent interface/api behavior.
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.
who required this? Well don't listen them. As long as two developer approve a PR. It gets merged (with no issues and change requests).
if (maxSizeWhenAll0xff > 0) | ||
seekPrefix = ((byte)0xff).Repeat(maxSizeWhenAll0xff); | ||
else | ||
throw new NotSupportedException("Array filled with max value (0xFF)", new ArgumentException(nameof(keyPrefix))); |
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.
Your already using comparer
!!! What's the problem?
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.
Your already using
comparer
!!! What's the problem?
If compatibility of interface behavior is not required, this exception can be removed.
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.
Your already using
comparer
!!! What's the problem?
Becuase keyPrefix
is [0xff...] and direaction
is backword.
The seekPrefix
must greater than the last key in the store.
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 all stores and caches must function the same 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.
It all stores and caches must function the same way.
Which 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.
The way all stores work is, they look for a key [0xff, 0xff, 0xff, 0xff, 0xff]
'; for example, than returns that key if found, than everything from that point to the 1st key/record.
I know the key isn't all 0xff
in this example. But if that key exists with all 0xff
it would start there and go backwards.
Core Example:
neo/src/Plugins/ApplicationLogs/Store/LogStorageStore.cs
Lines 187 to 208 in 805678d
public IEnumerable<ContractLogState> FindContractState(UInt160 scriptHash, uint page, uint pageSize) | |
{ | |
var prefix = new KeyBuilder(Prefix_Id, Prefix_Contract) | |
.Add(scriptHash) | |
.ToArray(); | |
var prefixKey = new KeyBuilder(Prefix_Id, Prefix_Contract) | |
.Add(scriptHash) | |
.AddBigEndian(ulong.MaxValue) // Get newest to oldest (timestamp) | |
.ToArray(); | |
uint index = 1; | |
foreach (var (key, value) in _snapshot.Seek(prefixKey, SeekDirection.Backward)) // Get newest to oldest | |
{ | |
if (key.AsSpan().StartsWith(prefix)) | |
{ | |
if (index >= page && index < (pageSize + page)) | |
yield return value.AsSerializable<ContractLogState>(); | |
index++; | |
} | |
else | |
yield break; | |
} | |
} |
Note: It took me some time to figure this out how backwards
works. Think of it as an integer
and you're doing currentRecordKey <= Int.MaxValue
.
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 way all stores work is, they look for a key
[0xff, 0xff, 0xff, 0xff, 0xff]
'; for example, than returns that key if found, than everything from that point to the 1st key/record.I know the key isn't all
0xff
in this example. But if that key exists with all0xff
it would start there and go backwards.Core Example:
neo/src/Plugins/ApplicationLogs/Store/LogStorageStore.cs
Lines 187 to 208 in 805678d
public IEnumerable<ContractLogState> FindContractState(UInt160 scriptHash, uint page, uint pageSize) { var prefix = new KeyBuilder(Prefix_Id, Prefix_Contract) .Add(scriptHash) .ToArray(); var prefixKey = new KeyBuilder(Prefix_Id, Prefix_Contract) .Add(scriptHash) .AddBigEndian(ulong.MaxValue) // Get newest to oldest (timestamp) .ToArray(); uint index = 1; foreach (var (key, value) in _snapshot.Seek(prefixKey, SeekDirection.Backward)) // Get newest to oldest { if (key.AsSpan().StartsWith(prefix)) { if (index >= page && index < (pageSize + page)) yield return value.AsSerializable<ContractLogState>(); index++; } else yield break; } } Note: It took me some time to figure this out how
backwards
works. Think of it as aninteger
and you're doingcurrentRecordKey < Int.MaxValue
.
SeekDireaction.Bacworkd
makes the logic more complicated.
You can read this function(and previous version) at first.
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.
For now I'll approve. Because I'll be fixing these issue in future PR.
this IReadOnlyStoreView view, | ||
byte[]? keyPrefix, | ||
byte[]? seekPrefix, | ||
SeekDirection direction = SeekDirection.Forward) |
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.
Just use comparer
. Just forget about these direction things for a min. dotnet
uses comparer
why can't we? There are good reasons to use comparer
instead of enums
.
if (keyPrefix == null || key.ToArray().AsSpan().StartsWith(keyPrefix)) | ||
yield return new(key, value); | ||
else if (direction == SeekDirection.Forward || (seekPrefix == null || !key.ToArray().SequenceEqual(seekPrefix))) | ||
yield break; |
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.
Wouldn't have to do this if you use comparer
. Just look at LevelDbStore
and MemoryStore
.
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.
Wouldn't have to do this if you use
comparer
. Just look atLevelDbStore
andMemoryStore
.
These lines aren't new.
I moved these from DataCache.cs
to here.
https://github.com/neo-project/neo/blob/master/src/Neo/Persistence/DataCache.cs#L242
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.
Copying/Moving bugs doesn't make it ok.
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.
Copying/Moving bugs doesn't make it ok.
This PR didn't change this.
You can submit a new PR to fix it.
: ByteArrayComparer.Reverse; | ||
foreach (var (key, value) in view.Seek(inclusiveStartKey, direction)) | ||
{ | ||
if (comparer.Compare(key.ToArray(), exclusiveEndKey) < 0) |
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.
Where are you getting this information? What is not to understand? IStore
doesn't work this way. Find
has bugs in DataCache
.
if @shargon and @Jim8y is good with this PR can @superboyiii test? |
I'm not against this pr, just need more time to review it well |
Some problems already exist, which are not introduced by this PR.
|
@cschuchardt88 for issues that are not related i suggest u to open another pr to fix, as to some english or minor issues maybe you can also fix in another pr, that will make ur ideas being applied faster. |
I don't see the point of moving functions around. If you are to do that, at least fix the bugs in them. Put a little effort in, same with design. This isn't your own project, where its only used by you. Others have to support your copy and paste efforts. So some respect to code implementation would be nice. Why am I always the one to clean up everyones shit show? |
|
Ill answer in order:
|
Description
Add scan operations for
IReadOnlyStoreView
ScanPrefix
andScanRange
are clearer thanFind
, andFindRange
A better way to fix #3681
Type of change
Checklist: