-
Notifications
You must be signed in to change notification settings - Fork 465
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
Declarative RLP Encoding/Decoding #7975
base: master
Are you sure you want to change the base?
Changes from 2 commits
d3a3232
9a90ccb
30dda91
d3d83f9
65f0891
b4656cd
a63b64f
6735d29
74cf2fd
2ef4c4d
b3dddcb
8b19e01
2c67165
52e1f7b
7262407
e4c2e63
35b9203
e0ec1ce
d3a2e5b
67a69df
dc3798b
ffa62f4
e626fc9
6f3efbc
2d2eeda
67abc2b
8f4b02a
9db2ebb
2223845
51fd038
5fad6ef
c22f1df
70c78e2
f11ff9a
38cbb3d
f284c28
6e91e0d
ec2fdbc
f158463
6677222
e32a33c
f665779
2e75a7d
aa6b4bd
fcde607
416a45b
837c2e4
f484b8b
599da39
3eff5ea
b7889ed
d41ecd3
03325a0
914505a
7dbc2dc
15eb6b6
c42eb50
ed0d88b
88a8cc4
a596ab0
df3ffc9
42b4401
917a7b8
88e7bac
1bc2d38
fb254e9
21a18b9
4740466
5705da1
cc58776
2456c40
868d272
69c8a92
f0191d1
1011df9
63c3ed9
0c2807b
410dfc7
efc25bd
aae0100
c7a32d1
3dc5b65
fd9fc30
9f5d236
8341a0e
3e1330d
4f68cb3
ac98b21
79dc13f
c9ba2eb
e43f13c
db5408a
38eb0f0
31726b6
149145b
dd57f80
40cfd25
f419316
d3592d8
a52d923
606a260
abf143d
2a9bee7
d0dfb12
bcaa83f
783acc6
6dc21f4
1c8f472
b2e5951
9622a7b
11bf0c7
92bc902
7e03ac3
04ac6b2
3250b15
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 |
---|---|---|
|
@@ -3,31 +3,27 @@ | |
|
||
using System; | ||
using System.Buffers; | ||
using System.Runtime.CompilerServices; | ||
using System.Diagnostics; | ||
|
||
namespace Nethermind.Serialization.FluentRlp; | ||
|
||
public static class Rlp | ||
{ | ||
public static ReadOnlyMemory<byte> Write(RefRlpWriterAction action) | ||
public static byte[] Write(RefRlpWriterAction action) | ||
=> Write(action, static (ref RlpWriter w, RefRlpWriterAction action) => action(ref w)); | ||
|
||
public static ReadOnlyMemory<byte> Write<TContext>(TContext ctx, RefRlpWriterAction<TContext> action) | ||
public static byte[] Write<TContext>(TContext ctx, RefRlpWriterAction<TContext> action) | ||
where TContext : allows ref struct | ||
{ | ||
var lengthWriter = RlpWriter.LengthWriter(); | ||
action(ref lengthWriter, ctx); | ||
var buffer = new ArrayBufferWriter<byte>(lengthWriter.Length); | ||
var contentWriter = RlpWriter.ContentWriter(buffer); | ||
var bufferWriter = new FixedArrayBufferWriter<byte>(lengthWriter.Length); | ||
var contentWriter = RlpWriter.ContentWriter(bufferWriter); | ||
action(ref contentWriter, ctx); | ||
|
||
return buffer.WrittenMemory; | ||
return bufferWriter.Buffer; | ||
} | ||
|
||
public static T Read<T>(ReadOnlyMemory<byte> source, RefRlpReaderFunc<T> func) where T : allows ref struct | ||
=> Read(source.Span, func); | ||
|
||
[OverloadResolutionPriority(1)] | ||
public static T Read<T>(ReadOnlySpan<byte> source, RefRlpReaderFunc<T> func) | ||
where T : allows ref struct | ||
{ | ||
|
@@ -37,3 +33,43 @@ public static T Read<T>(ReadOnlySpan<byte> source, RefRlpReaderFunc<T> func) | |
return result; | ||
} | ||
} | ||
|
||
/// <remarks> | ||
/// The existing <see cref="ArrayBufferWriter{T}"/> performs various bound checks and supports resizing buffers | ||
/// which we don't need for our use case. | ||
/// </remarks> | ||
internal class FixedArrayBufferWriter<T> : IBufferWriter<T> | ||
{ | ||
private readonly T[] _buffer; | ||
private int _index; | ||
|
||
public T[] Buffer => _buffer; | ||
|
||
/// <summary> | ||
/// Creates an instance of an <see cref="FixedArrayBufferWriter{T}"/>, in which data can be written to, | ||
/// with a fixed capacity specified. | ||
/// </summary> | ||
/// <param name="capacity">The capacity of the underlying buffer.</param> | ||
public FixedArrayBufferWriter(int capacity) | ||
{ | ||
_buffer = new T[capacity]; | ||
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. ArrayPool? 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. By default I think we should go with a plain array to match the If we want more control over the buffers we use we can write a custom |
||
_index = 0; | ||
} | ||
|
||
public void Advance(int count) | ||
{ | ||
_index += count; | ||
} | ||
|
||
public Memory<T> GetMemory(int sizeHint = 0) | ||
{ | ||
Debug.Assert(_buffer.Length > _index); | ||
return _buffer.AsMemory(_index); | ||
} | ||
|
||
public Span<T> GetSpan(int sizeHint = 0) | ||
{ | ||
Debug.Assert(_buffer.Length > _index); | ||
return _buffer.AsSpan(_index); | ||
} | ||
} |
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.
When is it used? Do we write the data 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.
When is what used? We "write" the data twice to first compute the length (
LengthWriter
), and then we actually write the bytes into a buffer (ContentWriter
).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 costly. There might be a way to write I think. Paprika does it. In Paprika, there's enough room left at the beginning to encode any length, then once the encoding is done, the length is written at the beginning and the position is adjusted. This would require a copy of the memory though as it would leave a gap at the beginning. This might be still cheaper if you copy the memory once more instead of going through the serialization two times?
https://github.com/NethermindEth/Paprika/blob/61c326a39e942ab9c550f6a6b9b5df59e67c0f0f/src/Paprika/Merkle/ComputeMerkleBehavior.cs#L750-L757
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 could work but I think it would be quite a redesign if it were to be adapted to all paths, not only here at the top level. Recursive calls in
WriteSequence
would need to be adjusted to first immediately write the bytes and then move them around to deal with writing the length first.I don't have an immediate answer as to what approach is cheaper. Moving the data around sounds more expensive (to me at least) than the current approach but it might we worth exploring.
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.
Moving once vs writing twice 👀 Clearly, I'm just asking as I don't have a clear answer. Also, in Paprika there are just a few cases where it's needed as majority of the written RLP is really short (beside branches, where it's applied).