search

RuedigerMoeller / fast-serialization

FST: fast java serialization drop in-replacement
Apache License 2.0
1.59k stars 247 forks source link

Fix FSTInputStream reading old data from the buffer #208

Closed carterkozak closed 7 years ago

carterkozak commented 7 years ago

ensureReadAhead previously did not error when it read fewer than the requested number of bytes.

Added new FSTDecoder.attemptReadAhead(int byte) method to prebuffer if possible, where ensureReadAhead will throw if the requested number of bytes cannot be read.

carterkozak commented 7 years ago

I had an issue with streams ending prematurely causing FST to read (stale) data from the internal buffer, from a different object. This change should help avoid some foot-shooting :-)

RuedigerMoeller commented 7 years ago

Hi, thanks for your work :).

Note: the ensureReadAhead is the most called method of fst and very prone to performance degradation.

I benchmarked the effects using https://github.com/RuedigerMoeller/serialization-benchmark (run TestRunner)

Unfortunately your changes create an intolerable slowdown:

some examples: image can you provide a reproducer so we might figure out a more performance sensitive way to fix this ?

EDIT: slowdown times are is readtime only

carterkozak commented 7 years ago

Ouch, that is staggering!

The issues I'm seeing seem to arise when the provided InputStream ends prematurely (or throws an exception with FSTInputStream.REPORT_READ_FAILS=false).

In my case I'm doubtful that this was the cause of my problem, but makes debugging the root cause more difficult because it surfaced as an array index out of bounds exception.

It may be possible to craft a request that forces FST to read data from the buffer from an entirely different request!