[wip] sdk perf investigation #208
Conversation
| @@ -32,7 +33,11 @@ export function encodeMessage(msg: Message): Uint8Array { | |||
| } | |||
|
|
|||
| export function encodeMessages(messages: Message[]): Uint8Array { | |||
| const chunks: Buffer[] = []; | |||
There was a problem hiding this comment.
I wonder why Buffer allocates so much then
| const headers = []; | ||
| let off = 0; | ||
|
|
||
| const writer = _m0.Writer.create(); |
There was a problem hiding this comment.
In both the rust and java apis for protobuf there's a way to estimate the size of the message, so you can directly allocate a buffer of message_size + 8. Is it possible here?
| // BUT we must wait for the previous flush to end. | ||
| this.flushing = this.flushing.then(() => this.scheduleFlush()); | ||
| } | ||
| // tag along to the previously scheduled flush. |
There was a problem hiding this comment.
Maybe change this comment in something more explicative, e.g.:
// we don't need to reschedule the `flush` here,
// because the flush happens anyway at the end of the current event loop iteration.
| this.sdkOutput = rawStream; | ||
|
|
||
| this.outputBuffer = new BufferedConnection((buffer) => { | ||
| const hasMoreCapacity = rawStream.write(buffer); |
There was a problem hiding this comment.
is this hasMoreCapacity something we need for correctness? could we just get rid of this and simply do rawStream.write?
There was a problem hiding this comment.
I wanted to keep the current semantics which is also guarded by the tests.
I think these layers need some attention whatever time permits.
There was a problem hiding this comment.
But technically you are right
There was a problem hiding this comment.
Ok then maybe let's track this in a separate issue.
| @@ -201,23 +207,16 @@ export class RestateHttp2Connection implements Connection { | |||
| * capacity again, so that at least the operations that await results will respect backpressure. | |||
| */ | |||
| public send(msg: Message): Promise<void> { | |||
There was a problem hiding this comment.
If this promise is never used, can we change the send signature here to be void?
There was a problem hiding this comment.
I'd rather to keep it as is for now, keep the change minimal.
igalshilman
force-pushed
the
sdk_perf
branch
2 times, most recently
from
e0b3ac1 to
d8778e0
Compare
This commit overall improves the performances of sending journal entries to the runtime by 4x, by reducing the number of temporary buffer allocations, promises, and intermediate serialization.
igalshilman
force-pushed
the
sdk_perf
branch
from
d8778e0 to
a18b44c
Compare
No description provided.