Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-2021

Improve large binary protocol string performance

Details

    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.9
    • 0.10.0
    • C++ - Library
    • None
    • Patch Available

    Description

      Currently, TBinaryProtocol reads into a temporary buffer, then copies the results into the resulting std::string. For large strings (like a megabyte), the copy can be a performance issue.

      The attached patch reads directly into the std::string instead.

      Attachments

        1. fast_binary_strings.patch
          1.0 kB
          Ben Craig

        Activity

          roger Roger Meier added a comment -

          Hmm... I think we should keep "prevent stack overflow for v. large strings"

          roger Roger Meier added a comment - Hmm... I think we should keep "prevent stack overflow for v. large strings"
          ben.craig Ben Craig added a comment -

          For large strings, the heap is being used in both cases. std::string generally uses the heap, and only small strings go on the stack.

          If there is a heap overflow issue, then both the old and new code are equally vulnerable, as both call this->trans->readAll into a buffer that could be exactly "size" bytes. I don't think there's a heap overflow in either set of code, but if there were, then the new code shouldn't make it any worse.

          ben.craig Ben Craig added a comment - For large strings, the heap is being used in both cases. std::string generally uses the heap, and only small strings go on the stack. If there is a heap overflow issue, then both the old and new code are equally vulnerable, as both call this->trans->readAll into a buffer that could be exactly "size" bytes. I don't think there's a heap overflow in either set of code, but if there were, then the new code shouldn't make it any worse.
          jensg Jens Geyer added a comment -

          +1
          Applies after fixing EOL style (win=>*nix), looks good to me.

          jensg Jens Geyer added a comment - +1 Applies after fixing EOL style (win=>*nix), looks good to me.
          hudson Hudson added a comment -

          SUCCESS: Integrated in Thrift #926 (See https://builds.apache.org/job/Thrift/926/)
          THRIFT-2021: Improve large binary protocol string performance (bencraig: rev fd64c15c4fa5ab092ecdda713bae142c05aafd72)

          • lib/cpp/src/thrift/protocol/TBinaryProtocol.tcc
          hudson Hudson added a comment - SUCCESS: Integrated in Thrift #926 (See https://builds.apache.org/job/Thrift/926/ ) THRIFT-2021 : Improve large binary protocol string performance (bencraig: rev fd64c15c4fa5ab092ecdda713bae142c05aafd72) lib/cpp/src/thrift/protocol/TBinaryProtocol.tcc

          People

            ben.craig Ben Craig
            ben.craig Ben Craig
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: