[FLINK-35215][core] Fix the bug when Kryo serialize length is 0

[1996fanrui]

What is the purpose of the change

The performance of serializerKryo and serializerKryoWithoutRegistration are regressed, I checked recent commits, and found FLINK-34954 changed related logic.

Brief change log

• The first commit: Revert "[FLINK-34954][core] Kryo Input bug fix"
• The second commit: [FLINK-35215][core] Fix the bug when Kryo serialize length is 0
  • This PR reverts the FLINK-34954, and try to re-implement to avoid the performance regression.

[flinkbot]

CI report:

• ba2740ff65e16c7eb9eb09a6c620c5722e63e78b Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[reswqa]

Not to block this, just curious do you know why FLINK-34954 affects performance?

[kkrugler]

I'm leaving in 2 days for a 4+ month backpack trip (hiking the PCT), otherwise what I'd do is try to hook up a test that uses JMH to accurately assess the impact of the change. To respond to the particular comment on why performance changed:

we call bytesRead < count twice

The overhead of two comparisons, versus even one call to inputStream.read, should be miniscule.

I'm not sure whether FLINK-34954 breaks any JIT optimization

If a JIT optimization would result in skipping calls to inputStream.read, then yes that could have an impact. But that would be a change in logic.

The one thing I see is in the change to readBytes(), where if it's often called with a count of 0, then your change skips the try/catch block, which is non-trivial. But I thought the fix was to avoid a failure for this exact case, so it shouldn't be happening in the current benchmark code.

[1996fanrui]

I'm leaving in 2 days for a 4+ month backpack trip (hiking the PCT)

Wow, have a nice trip!

To respond to the particular comment on why performance changed:

we call bytesRead < count twice

The overhead of two comparisons, versus even one call to inputStream.read, should be miniscule.

I'm not sure whether FLINK-34954 breaks any JIT optimization

If a JIT optimization would result in skipping calls to inputStream.read, then yes that could have an impact. But that would be a change in logic.

As I said before, I'm not sure the reason, and both of them are my guess.

The one thing I see is in the change to readBytes(), where if it's often called with a count of 0, then your change skips the try/catch block, which is non-trivial. But I thought the fix was to avoid a failure for this exact case, so it shouldn't be happening in the current benchmark code.

Sorry, I didn't fully understand this sentence. Do you mean skip the try/catch block is not fine?

[kkrugler]

Sorry for the confusing sentence. I meant that skipping the try/catch block would save a lot of cycles, but your change that causes it to be (correctly) skipped when the count is 0 shouldn't be the common case, so I wouldn't expect that to impact the benchmark results.

[1996fanrui]

Sorry for the confusing sentence. I meant that skipping the try/catch block would save a lot of cycles, but your change that causes it to be (correctly) skipped when the count is 0 shouldn't be the common case, so I wouldn't expect that to impact the benchmark results.

Thanks for the quick clarification! Yes, my change only adds a simple check if (count == 0) return;, and it doesn't impact performance after I run the benchmark.