Thrift
  1. Thrift
  2. THRIFT-1457

Capacity of TframedTransport write buffer is never reset

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.6.1
    • Fix Version/s: 0.9.2
    • Component/s: Java - Library
    • Labels:
      None

      Description

      The writeBuffer_ instance variable of TframedTransport can only grow and is never reset to its original capacity. This causes memory issues for clients using a connection pool.

      The size of this buffer grows to 16MB in our application. With a connection pool of 50 connections, this leads to a loss up to 800MB.

      1. THRIFT-1457.patch
        0.9 kB
        Arthur Meyer

        Activity

        Hide
        Arthur Meyer added a comment -

        attached possible patch

        Show
        Arthur Meyer added a comment - attached possible patch
        Hide
        Bryan Duxbury added a comment -

        Because allocating write buffers takes time, there are a lot of situations where it's desirable for a buffer to never shrink just to avoid the possibility of having to reallocate it later. Your situation is the opposite, where you'd actually prefer to save the memory at the expense of time. I think for us to accept a patch for this issue, we would have to support both strategies.

        Show
        Bryan Duxbury added a comment - Because allocating write buffers takes time, there are a lot of situations where it's desirable for a buffer to never shrink just to avoid the possibility of having to reallocate it later. Your situation is the opposite, where you'd actually prefer to save the memory at the expense of time. I think for us to accept a patch for this issue, we would have to support both strategies.
        Hide
        Arthur Meyer added a comment -

        I understand your concerns. How do you propose to support both use strategies?

        I could add an extra constructor with an extra parameter to specify the strategy to use.
        Based on the parameter the buffer will be either reset or reused.

        I'm not sure how to support both strategies in the code that uses TByteArrayOutputStreams.
        Could you give me some pointers?

        Show
        Arthur Meyer added a comment - I understand your concerns. How do you propose to support both use strategies? I could add an extra constructor with an extra parameter to specify the strategy to use. Based on the parameter the buffer will be either reset or reused. I'm not sure how to support both strategies in the code that uses TByteArrayOutputStreams. Could you give me some pointers?
        Hide
        Bryan Duxbury added a comment -

        You could just implement another framed transport that discards buffers instead of reuses them...

        Show
        Bryan Duxbury added a comment - You could just implement another framed transport that discards buffers instead of reuses them...
        Hide
        Roger Schildmeijer added a comment -

        Arthur Meyer did you manage to get around this? I've been bitten by this when using a popular Cassandra java client.

        Show
        Roger Schildmeijer added a comment - Arthur Meyer did you manage to get around this? I've been bitten by this when using a popular Cassandra java client.
        Hide
        Arthur Meyer added a comment -

        We use a patched version of libthrift-0.7.0.jar containing the attached patch for TByteArrayOutputStream.
        Most of our queries have a small resultset, so the overhead of reallocation isn’t significant.

        Show
        Arthur Meyer added a comment - We use a patched version of libthrift-0.7.0.jar containing the attached patch for TByteArrayOutputStream. Most of our queries have a small resultset, so the overhead of reallocation isn’t significant.
        Hide
        Liang Xie added a comment -

        Implement another framed transport seems a little heavy, right, how about let's introduce a config parameter(e.g. reuseWriteBuffer) in TFramedTransport ?

        Show
        Liang Xie added a comment - Implement another framed transport seems a little heavy, right, how about let's introduce a config parameter(e.g. reuseWriteBuffer) in TFramedTransport ?
        Hide
        Derek Lewis added a comment -

        We've also applied this patch to a internally forked libthrift.

        Show
        Derek Lewis added a comment - We've also applied this patch to a internally forked libthrift.
        Hide
        Roger Meier added a comment -

        committed

        Show
        Roger Meier added a comment - committed
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Thrift #1036 (See https://builds.apache.org/job/Thrift/1036/)
        THRIFT-1457 java: Capacity of TframedTransport write buffer is never reset (roger: rev 213ea258de6796e1302e57f4246434ca3c9a2ac6)

        • lib/java/src/org/apache/thrift/TByteArrayOutputStream.java
        Show
        Hudson added a comment - FAILURE: Integrated in Thrift #1036 (See https://builds.apache.org/job/Thrift/1036/ ) THRIFT-1457 java: Capacity of TframedTransport write buffer is never reset (roger: rev 213ea258de6796e1302e57f4246434ca3c9a2ac6) lib/java/src/org/apache/thrift/TByteArrayOutputStream.java

          People

          • Assignee:
            Roger Meier
            Reporter:
            Arthur Meyer
          • Votes:
            1 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development