Improve binary read performance

[kbongort]

This PR makes a couple of improvements to binary read performance:

1. One of the slowest things during a read with many messages is initializing a BinaryReader for each message with its DataView and TextDecoder. This PR adds a messageField() method to BinaryReader that reads a size-delimited message field without constructing a new BinaryReader. (Message.fromBinary is refactored to share code). A knock-on effect of this change is that we pass options along without repeated calls to makeReadOptions, which otherwise needlessly spreads options into readDefaults even when options is already identical to readDefaults.
2. readScalar was calling reader[method]() which seems to incur an implicit call to .bind() on the reader method (i.e. it is equivalent to reader[method].bind(reader)()). By using an inline switch statement we can avoid that indirection and use a faster path.

I put together a simple benchmark (https://github.com/kbongort/protobuf-es/pull/1) using the following message definitions:

message User {
  string first_name = 1;
  string last_name = 2;
  bool active = 3;
  User manager = 4;
  repeated string locations = 5;
  map<string, string> projects = 6;
  int32 number = 7;
}

message UserList {
  repeated User users = 1;
}

The benchmark uses both a more complete version of the User message and one with only a few fields set:

const userMessage = new User({
  firstName: "Jane",
  lastName: "Doe",
  active: true,
  manager: { firstName: "Jane", lastName: "Doe", active: false },
  locations: ["Seattle", "New York", "Tokyo"],
  projects: { foo: "project foo", bar: "project bar" },
});

const smallUserMessage = new User({
  manager: { active: true },
  number: 2,
});

After warming up the cache by reading a serialized userMessage 1000 times, it measures the time to read userMessage, to read a UserList of 1000 userMessages, and to read a UserList of 1000 smallUserMessages. The amount of time it takes to call JSON.parse() on the JSON version of the message is provided as a point of comparison. Without the changes in this PR, the results are:

Benchmark: Single user
	Time to parse JSON: 1135 µs
	Time to parse from binary: 3548 µs
	Ratio: 312%
Benchmark: List of 1000 users
	Time to parse JSON: 920 ms
	Time to parse from binary: 2957 ms
	Ratio: 321%
Benchmark: List of 1000 small users
	Time to parse JSON: 260 ms
	Time to parse from binary: 923 ms
	Ratio: 355%

After the changes in this PR, the results are:

Benchmark: Single user
	Time to parse JSON: 1088 µs
	Time to parse from binary: 2755 µs
	Ratio: 253%
Benchmark: List of 1000 users
	Time to parse JSON: 921 ms
	Time to parse from binary: 1729 ms
	Ratio: 188%
Benchmark: List of 1000 small users
	Time to parse JSON: 255 ms
	Time to parse from binary: 237 ms
	Ratio: 93%

The three test cases show speedups of 1.3x, 1.7x, and 3.9x, respectively. The effect of (1) is especially pronounced when there are many small messages, as in the last benchmark test.

[CLAassistant]

All committers have signed the CLA.

[smaye81]

Hi @kbongort appreciate your patience on this. I know we spoke offline last week. We have been gathering some baseline metrics on our performance to see all the areas that need addressed. I can confirm that this does improve binary read performance by an average of ~2.5x.

However, one concern is the change to modify the IBinaryReader interface. This is an interface we expose from the library and changing it will result in breaking changes. Are you up for potentially modifying the PR so that these changes can be made in a non-breaking way without affecting the interface contract?

[kbongort]

Hi @kbongort appreciate your patience on this. I know we spoke offline last week. We have been gathering some baseline metrics on our performance to see all the areas that need addressed. I can confirm that this does improve binary read performance by an average of ~2.5x.

However, one concern is the change to modify the IBinaryReader interface. This is an interface we expose from the library and changing it will result in breaking changes. Are you up for potentially modifying the PR so that these changes can be made in a non-breaking way without affecting the interface contract?

What would you suggest?

[smaye81]

Let me talk it over with the team. We know that the fromBinary > readMessage path is a bottleneck, but I'm not sure if there is a good way to reuse the BinaryReader like you're doing without breaking one of the interfaces.

In the meantime, would you mind splitting out the two improvements (BinaryReader vs. readScalar). I think we can probably get the latter implemented and at least get a quick win.

[kbongort]

Let me talk it over with the team. We know that the fromBinary > readMessage path is a bottleneck, but I'm not sure if there is a good way to reuse the BinaryReader like you're doing without breaking one of the interfaces.

In the meantime, would you mind splitting out the two improvements (BinaryReader vs. readScalar). I think we can probably get the latter implemented and at least get a quick win.

What type of breakage are you concerned with, exactly? This does add a couple of methods to IBinaryReader. Are you worried that clients may have IBinaryReader implementations that will no longer conform to the interface, or something else?

[kbongort]

I've updated the PR to not modify IBinaryReader. Since the new methods are implemented strictly in terms of existing ones, it's possible to refactor them into static utility methods. So that's what I did.

[kbongort]

@smaye81 PTAL – Let me know if you have any further concerns, thanks!

[kbongort]

Hi Steve, I believe that I've addressed your concerns with this PR. Can you please take another look? Thanks, Ken

…

On Tue, May 9, 2023 at 3:13 PM Steve Ayers ***@***.***> wrote: Let me talk it over with the team. We know that the fromBinary > readMessage path is a bottleneck, but I'm not sure if there is a good way to reuse the BinaryReader like you're doing without breaking one of the interfaces. In the meantime, would you mind splitting out the two improvements ( BinaryReader vs. readScalar). I think we can probably get the latter implemented and at least get a quick win. — Reply to this email directly, view it on GitHub <#459 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGSDEQCO6KDTS7CIJNFVVTXFK6O5ANCNFSM6AAAAAAXAYRMIA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[smaye81]

Hi @kbongort -- yes, apologies for the delay. we have been a bit heads-down on some other work at the moment, but will take a look at this hopefully this week. thanks again for the PR.

cc @timostamm

[timostamm]

Hey @kbongort 👋

We added basic performance benchmarks in #491. They confirm what you already measured, just provide a little bit more detail.

The downside of keeping the BinaryReader is that it makes it more difficult to generate speed-optimized code, which is the best lever for performance we have. But looking at the change in isolation, it does speed up the test "large google.protobuf.FileDescriptorSet" by ~35%. This seems worth it.

I pushed up 126ba29 to avoid T.fromBinary() in readMapEntry(), and 77460d3 to move BinaryReaderUtil.readMessage() to a local function. It keeps the performance improvement, and reduces the bundle size increase to negligible 0.02%.

The readScalar() change is a 1% increase in bundle size (that's the reason it wasn't a switch statement in the first place). But it's a really decent performance bump with 55% for "large google.protobuf.FileDescriptorSet". This is worth it. Good catch 🙂

Thank you for the contribution, LGTM!

[kbongort]

Thanks for the review & further work! 🍾