Save state before using BinaryWriter

[Demon000]

This allows returning the same BinaryWriter instance for all calls inside writerFactory().

This does not break previous implementations of IBinaryWriter which do not flush the state inside finish() since they did not work correctly when trying to write nested messages with the same instance anyway.

[CLAassistant]

All committers have signed the CLA.

[timostamm]

@Demon000, can you provide some information about the use case? Is this about a performance improvement, or something else?

[Demon000]

Yes, this is supposed to be an opt-in performance improvement. Opting in is done by returning the same BufferWriter instance inside writerFactory().

Another reason for which someone might want this change is if the BufferWriter implementation has some logic that only needs to be ran once per every encode. In my case, I want to reserve a few bytes at the beginning of the buffer to allow some data to be added as a header, without having to do an extra buffer allocation and copy afterwards.

Without these fixes, the generated binary data is not proper.

[timostamm]

Thanks for the PR, @Demon000. We have to be very careful with changes to this part of the code base. It looks like we can achieve the same thing with a much more simple change, see comments below.

If you want to make that update, we're happy to accept.

[timostamm]

The documentation states that the method "resets this writer". It looks like the function does not reset the property buf . It makes sense to me to fix this up, and add this.buf = [] here.

From what I see, this change would be sufficient for your use case, and it would improve the behavior of the writer. I do not think that any other changes should be made for this fix, at least not without adding significant test coverage first and taking a close look at the impact on performance.

[timostamm]

Regarding performance, I would much prefer that we take the same approach as #459 did for binary read. The impact was pretty big, and I guess it will be similar when doing the same for binary read.

There are three calls to Message.toBinary() in binary-format-common.ts, for example this one. If they are replaced with calls to runtime.bin.writeMessage, we don't create the options object multiple times (and we also don't create multiple writers).

In the end, this perf improvement will also enable your use case, since writerFactory will only be called once, but I think it makes sense to treat the two issues separately, and to validate the assumptions about the perf improvement with benchmarks.

[timostamm]

Closing, but please feel free to re-open with the requested changes 🙂