nats-io / nats-server

High-Performance server for NATS.io, the cloud and edge native messaging system.
https://nats.io
Apache License 2.0
16.03k stars 1.41k forks source link

[v2.11] Reduce cost of json Unmarshal operations #5956

Closed Jarema closed 1 month ago

Jarema commented 1 month ago

Until now, in JS API we were checking if request payload is empty. That check was unmarshaling that payload before we did second unmarshal for the destination type. This commit adds separate check to avoid double unmarshaling.

This PR does not change isEmptyRequest implementation, as its a less strict check.

Signed-off-by: Tomasz Pietrek tomasz@nats.io

ripienaar commented 1 month ago

the PR is attempting to avoid this unmarshal here

https://github.com/nats-io/nats-server/blob/ef7d9125ed7bf3678eef482fce63625e06e9c683/server/jetstream_api.go#L2891

derekcollison commented 1 month ago

Ah gotcha! Thanks and apologies did not look at the complete function for isEmptyRequest.

derekcollison commented 1 month ago

Let's address @aricart comment then should be good to go. Thanks @Jarema !

wallyqs commented 1 month ago

There is json.Valid([]byte) in the stdlib, that would be lighter than the call to json.Unmarshal: https://pkg.go.dev/encoding/json#Valid

derekcollison commented 1 month ago

Should we use that instead @Jarema and just change isEmptyRequest?

ripienaar commented 1 month ago

The code isn’t checking if it’s valid json. It’s checking if the request - even if valid - is an empty object.

Jarema commented 1 month ago

As @ripienaar mentioned - json.Unmarshal in isEmptyRequest does not check just for validity.