Skip to content
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

Save state before using BinaryWriter #674

Closed
wants to merge 1 commit into from

Conversation

Demon000
Copy link

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.

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
Copy link

CLAassistant commented Jan 20, 2024

CLA assistant check
All committers have signed the CLA.

@timostamm
Copy link
Member

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

@Demon000
Copy link
Author

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.

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -329,15 +331,28 @@ export class BinaryWriter implements IBinaryWriter {
bytes.set(this.chunks[i], offset);
offset += this.chunks[i].length;
}
this.chunks = [];

this.restore();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

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

@timostamm timostamm closed this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants