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

Capacity of TframedTransport write buffer is never reset

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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
        arthurm Arthur Meyer added a comment -

        attached possible patch

        Show
        arthurm Arthur Meyer added a comment - attached possible patch
        Hide
        bryanduxbury 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
        bryanduxbury 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
        arthurm 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
        arthurm 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
        bryanduxbury Bryan Duxbury added a comment -

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

        Show
        bryanduxbury Bryan Duxbury added a comment - You could just implement another framed transport that discards buffers instead of reuses them...
        Hide
        rschildmeijer 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
        rschildmeijer 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
        arthurm 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
        arthurm 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
        xieliang007 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
        xieliang007 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
        lewisd Derek Lewis added a comment -

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

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

        committed

        Show
        roger.meier Roger Meier added a comment - committed
        Hide
        hudson 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 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 Roger Meier
            Reporter:
            arthurm Arthur Meyer
          • Votes:
            1 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development