Introduce a ThriftProtocolError to avoid allocating and formattings strings for error messages

[jhorstmann]

Which issue does this PR close?

This is a small performance improvement for the thrift remodeling

• Part of [EPIC] A collection of items to improve speed of parquet metadata encoding #5853.

Rationale for this change

Some of the often-called methods in the thrift protocol implementation created ParquetError instances with a string message that had to be allocated and formatted. This formatting code and probably also some drop glue bloats these otherwise small methods and prevented inlining.

What changes are included in this PR?

Introduce a separate error type ThriftProtocolError that is smaller than ParquetError and does not contain any allocated data. The ReadThrift trait is not changed, since its custom implementations actually require the more expressive ParquetError.

Are these changes tested?

The success path is covered by existing tests. Testing the error paths would require crafting some actually malformed files, or using a fuzzer.

Are there any user-facing changes?

The ThriftProtocolError is crate-internal so there should be no api changes. Some error messages might differ slightly.

[jhorstmann]

@alamb @etseidl I might have found another small performance improvement in the new thrift code :)

Need to fix the clippy issues and format the error messages a bit nicer before this can be merged.

[etseidl]

Thanks @jhorstmann! It's amazing how much time is spent on innocuous snippets of code. I've found that the skipping code spends a significant amount of time in the FieldType impl of ParitalEq, and have branch where I delay conversion to FieldType until all matching is complete.

[etseidl]

group                             pq_err                                 thrift_err
-----                             ------                                 ----------
decode parquet metadata           1.14      5.6±0.06µs        ? ?/sec    1.00      4.9±0.03µs        ? ?/sec
decode parquet metadata (wide)    1.32     23.0±0.17ms        ? ?/sec    1.00     17.5±0.20ms        ? ?/sec
open(default)                     1.13      5.8±0.05µs        ? ?/sec    1.00      5.2±0.03µs        ? ?/sec
open(page index)                  1.02    104.0±1.02µs        ? ?/sec    1.00    102.5±0.47µs        ? ?/sec

This is a small performance improvement

🤣 🤣 🤣

I think Andrew may have to redo the images for the blog post again 😂

[etseidl]

Thanks @jhorstmann, this is great.

[alamb]

I think Andrew may have to redo the images for the blog post again 😂

That is ok

There is also a report that the C++ thrift generated code is faster than this parser -- https://lists.apache.org/thread/skr7f2tf94q59cx390cq2sw8f1nps675

I haven't been able to reproduce that result yet

[alamb]

Love it. It is a fascinating observation that constructing errors with strings (even when they aren't constructed) can slow our code down

I wonder how many other codepaths in arrow/parquet have the same property 🤔

[etseidl]

There is also a report that the C++ thrift generated code is faster than this parser -- https://lists.apache.org/thread/skr7f2tf94q59cx390cq2sw8f1nps675

I haven't been able to reproduce that result yet

🤔

So I did a quick sanity check on my workstation.

  // 'buf' contains bytes for footer
  for (int i=0; i < 1000; i++) {
    std::shared_ptr<TMemoryBuffer> strBuf(new TMemoryBuffer(buf, ender.footer_len));
    TCompactProtocol proto{strBuf};
    parquet::format::FileMetaData fmd;
    fmd.read(&proto);
  }

vs

  // 'meta_data' contains bytes for footer
  for _ in 0..1000 {
    ParquetMetaDataReader::decode_metadata(&meta_data).unwrap();
  }

c++ time: 64.004u 8.960s 1:13.06 99.8% 0+0k 0+0io 0pf+0w
rust time: 26.714u 0.019s 0:26.77 99.8% 0+0k 0+0io 0pf+0w

This is with thrift-cpp 0.23

[jhorstmann]

Love it. It is a fascinating observation that constructing errors with strings (even when they aren't constructed) can slow our code down

It is, and I also don't really know how much of that is because of removing the error message formatting code, vs the smaller size of the error type. The former might just have influenced some heuristics around inlining.

I could imagine that C++ is actually smarter about eliminating redundant moves of such error structs.

I wonder how many other codepaths in arrow/parquet have the same property 🤔

I think you'd need a really high ratio of error handling vs "real" compute for that to have a measurable effect. Thrift parsing unfortunately needs error handling for nearly every byte, that shouldn't the case for other places in the code base.

Hmm, maybe the checked arithmetic kernels would actually also benefit from a separate error type.

[alamb]

So I did a quick sanity check on my workstation.

Thank you. I will sleep better tonight.

I wasn't able to find any reproduction instructions for their results but when they are able to provide them I plan to give it a good profile to see what is going on