Cassandra
  1. Cassandra
  2. CASSANDRA-5019

Still too much object allocation on reads

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Duplicate
    • Fix Version/s: 2.2.0 beta 1
    • Component/s: Core
    • Labels:

      Description

      ArrayBackedSortedColumns was a step in the right direction but it's still relatively heavyweight thanks to allocating individual Columns.

        Issue Links

          Activity

          Hide
          Benedict added a comment -

          Since the read path will be rewritten as part of efforts to introduce CASSANDRA-7447 (both regards internal APIs, and the implementation details for the new format), this ticket should be addressed by "doing things right" here. This may mean the legacy format continues to be somewhat inefficient, but this may or may not eventually be retired entirely, so there is probably not much point spending a lot of time optimising it, esp. when the impact is unknown and probably not dramatic in relation to the other costs associated with this format.

          Show
          Benedict added a comment - Since the read path will be rewritten as part of efforts to introduce CASSANDRA-7447 (both regards internal APIs, and the implementation details for the new format), this ticket should be addressed by "doing things right" here. This may mean the legacy format continues to be somewhat inefficient, but this may or may not eventually be retired entirely, so there is probably not much point spending a lot of time optimising it, esp. when the impact is unknown and probably not dramatic in relation to the other costs associated with this format.
          Hide
          Benedict added a comment -

          Ah, my mistake, I was looking at the CQL query processor. Either way, not to push the issue, but it's not absolutely necessary; an IncrementalRowSerializer (with an IncrementalColumnFamilySerializer) or similar could be defined which collects the atoms and serializes them immediately. Only needs to be used as an optimisation on the common code paths to make a big difference, and the remainder could simply invoke a wrapper method that constructs the Row as usual. The Row/ColumnFamily serializers could trivially be switched to calling these incremental serializers, so no code duplication, hot code paths can be updated without tremendous effort, and infrequent code paths take an imperceptible performance hit (given the context will be stack allocated it is unlikely to be measureable).

          Show
          Benedict added a comment - Ah, my mistake, I was looking at the CQL query processor. Either way, not to push the issue, but it's not absolutely necessary; an IncrementalRowSerializer (with an IncrementalColumnFamilySerializer) or similar could be defined which collects the atoms and serializes them immediately. Only needs to be used as an optimisation on the common code paths to make a big difference, and the remainder could simply invoke a wrapper method that constructs the Row as usual. The Row/ColumnFamily serializers could trivially be switched to calling these incremental serializers, so no code duplication, hot code paths can be updated without tremendous effort, and infrequent code paths take an imperceptible performance hit (given the context will be stack allocated it is unlikely to be measureable).
          Hide
          Jonathan Ellis added a comment -

          Look at ReadVerbHandler; we do need a Row to serialize.

          Show
          Jonathan Ellis added a comment - Look at ReadVerbHandler; we do need a Row to serialize.
          Hide
          Benedict added a comment -

          Major overhaul, so suggested with caution, but why don't we pass a context that knows what to do with the results, and is furnished with them as they are found by the CollationController? There's no real need to construct a Row at any point - in fact it looks like it gets translated straight into a List<List<ByteBuffer>> at the end anyway.

          Show
          Benedict added a comment - Major overhaul, so suggested with caution, but why don't we pass a context that knows what to do with the results, and is furnished with them as they are found by the CollationController? There's no real need to construct a Row at any point - in fact it looks like it gets translated straight into a List<List<ByteBuffer>> at the end anyway.
          Hide
          Jonathan Ellis added a comment -

          Hmm, I'm not sure what you're thinking of, but I'm talking about the main getColumnFamily -> CollationController path where we need to collect Columns into a ColumnFamily object so we can write them out to the wire. I don't think local Columns play a large role there. (If they did, couldn't we just avoid allocating a Column in the first place and just leave the raw fields on the stack?)

          Show
          Jonathan Ellis added a comment - Hmm, I'm not sure what you're thinking of, but I'm talking about the main getColumnFamily -> CollationController path where we need to collect Columns into a ColumnFamily object so we can write them out to the wire. I don't think local Columns play a large role there. (If they did, couldn't we just avoid allocating a Column in the first place and just leave the raw fields on the stack?)
          Hide
          Benedict added a comment - - edited

          We could just allocate a new flyweight Column object when we want to read it. In most cases the reference is unlikely to escape, and they'll be allocated on stack.

          Presumably in cases where they do escape we can either stop them from doing so or introduce some new method that converts them to a heavyweight Column. Not that this is likely to be a small task, having a look at it. Might be worth comparing performance with just this change and minimal optimisation work to see how it fairs, though?

          Show
          Benedict added a comment - - edited We could just allocate a new flyweight Column object when we want to read it. In most cases the reference is unlikely to escape, and they'll be allocated on stack. Presumably in cases where they do escape we can either stop them from doing so or introduce some new method that converts them to a heavyweight Column. Not that this is likely to be a small task, having a look at it. Might be worth comparing performance with just this change and minimal optimisation work to see how it fairs, though?
          Hide
          Jonathan Ellis added a comment -
          Show
          Jonathan Ellis added a comment - Possibly useful: https://github.com/RichardWarburton/slab
          Hide
          Jonathan Ellis added a comment -

          Want to give this a try, Carl Yeksigian?

          Show
          Jonathan Ellis added a comment - Want to give this a try, Carl Yeksigian ?
          Hide
          Jonathan Ellis added a comment -

          What makes this a bitch is that we ask our Iterators for Column objects, which we then add to the ColumnFamily. So we can't just drop in a FlyweightColumns CF subclass; the damage is already done by then.

          We'd have to push the CF into OnDiskAtomIterator and have it call add(name, value, timestamp) to avoid the Column construction overhead.

          Show
          Jonathan Ellis added a comment - What makes this a bitch is that we ask our Iterators for Column objects, which we then add to the ColumnFamily. So we can't just drop in a FlyweightColumns CF subclass; the damage is already done by then. We'd have to push the CF into OnDiskAtomIterator and have it call add(name, value, timestamp) to avoid the Column construction overhead.
          Hide
          Jonathan Ellis added a comment -

          Let's see if it's a benchmark win, if it is then we can go about trying to make it safe, e.g. w/ the kind of encapsulation I described.

          Show
          Jonathan Ellis added a comment - Let's see if it's a benchmark win, if it is then we can go about trying to make it safe, e.g. w/ the kind of encapsulation I described.
          Hide
          Sylvain Lebresne added a comment -

          I'm really not a fan. Doing that means we break the 'Column is an immutable object' assumption and it would make getting code right much harder. Typically, for a ColumnFamily backed byt such backing FlyweightColumns, you'd have to be extra careful when using it to never alias a Column returned during iteration because that alias would be invalidated by the next step of the iteration itself. I'm really afraid this would make it far too easy to get subtle bugs.

          Unless it can be demonstrated that such a thing brings concrete and substantial performance benefits, I think I'm -1 on it cause it would make imo the code too much fragile.

          Show
          Sylvain Lebresne added a comment - I'm really not a fan. Doing that means we break the 'Column is an immutable object' assumption and it would make getting code right much harder. Typically, for a ColumnFamily backed byt such backing FlyweightColumns, you'd have to be extra careful when using it to never alias a Column returned during iteration because that alias would be invalidated by the next step of the iteration itself. I'm really afraid this would make it far too easy to get subtle bugs. Unless it can be demonstrated that such a thing brings concrete and substantial performance benefits, I think I'm -1 on it cause it would make imo the code too much fragile.
          Hide
          Jonathan Ellis added a comment - - edited

          You're right. It would need to be more like this:

          class FlyweightColumns
          {
            ByteBuffer[] names;
            ByteBuffer[] values;
            long[] timestamps;
          
            private int index;
            private FlyweightColumn instance = new FlyweightColumn();
          
            public IColumn getColumn(int i)
            {
               index = i;
               return instance;
            }
          
            private class FlyweightColumn implements IColumn
            {
              public ByteBuffer name()
              {
                return names[index];
              }
          
              ... 
            }
          }
          

          Obviously some caution would be required (like ABSC, this is not suitable for Memtable or cache use) but this would be totally adequate for the main "read some columns for the user, then serialize them back to the coordinator" path.

          For more OO sugar we could move operations like "serialize columns to this OutputStream" into ISortedColumns instead of exposing getColumn(int) directly, I include that mainly because it is the easiest way to illustrate the FWC re-use.

          Show
          Jonathan Ellis added a comment - - edited You're right. It would need to be more like this: class FlyweightColumns { ByteBuffer[] names; ByteBuffer[] values; long [] timestamps; private int index; private FlyweightColumn instance = new FlyweightColumn(); public IColumn getColumn( int i) { index = i; return instance; } private class FlyweightColumn implements IColumn { public ByteBuffer name() { return names[index]; } ... } } Obviously some caution would be required (like ABSC, this is not suitable for Memtable or cache use) but this would be totally adequate for the main "read some columns for the user, then serialize them back to the coordinator" path. For more OO sugar we could move operations like "serialize columns to this OutputStream" into ISortedColumns instead of exposing getColumn(int) directly, I include that mainly because it is the easiest way to illustrate the FWC re-use.
          Hide
          Sylvain Lebresne added a comment -

          Actually, thinking about this issue a bit more, I'm wondering: what is it we expect to win here?

          Because we will still need to allocate one FlyweightColumn object per actual column. So in terms of sheer number of allocated objects, we'll have the same number (in fact we'll even have marginally more ojbects since we'll have to allocate the FlyweightColumns names, values and timestamps arrays).

          Now it's true that the memory layout will be slightly different, with the FlyweightColumn objects being slightly smaller than their Column counterpart (though overall we won't allocate less memory), but I'm not sure it makes much difference for the GC since the size difference is relatively minor. And on the other side, the circular dependency of FlyweightColumns to it's child FlyweightColumn might make the job of the GC a bit harder (it's more links to explore). Overall, it's far from obvious to me that it would improve things in a measurable way.

          And doing that would also mess a bit with the ColumnSerializer, as we would want it to create a flyweightColumn right away, meaning that it's not just an alternative ISortedColumn implementation, the abstraction would leak out.

          So am I missing something here and not understanding that idea at all?

          Show
          Sylvain Lebresne added a comment - Actually, thinking about this issue a bit more, I'm wondering: what is it we expect to win here? Because we will still need to allocate one FlyweightColumn object per actual column. So in terms of sheer number of allocated objects, we'll have the same number (in fact we'll even have marginally more ojbects since we'll have to allocate the FlyweightColumns names, values and timestamps arrays). Now it's true that the memory layout will be slightly different, with the FlyweightColumn objects being slightly smaller than their Column counterpart (though overall we won't allocate less memory), but I'm not sure it makes much difference for the GC since the size difference is relatively minor. And on the other side, the circular dependency of FlyweightColumns to it's child FlyweightColumn might make the job of the GC a bit harder (it's more links to explore). Overall, it's far from obvious to me that it would improve things in a measurable way. And doing that would also mess a bit with the ColumnSerializer, as we would want it to create a flyweightColumn right away, meaning that it's not just an alternative ISortedColumn implementation, the abstraction would leak out. So am I missing something here and not understanding that idea at all?
          Hide
          Sylvain Lebresne added a comment -

          And I don't think the replace we have is any problem for the flyweight pattern, since all we support is replacing a column by another with the same name. I.e. this is really an in-place replace only.

          Show
          Sylvain Lebresne added a comment - And I don't think the replace we have is any problem for the flyweight pattern, since all we support is replacing a column by another with the same name. I.e. this is really an in-place replace only.
          Hide
          Jonathan Ellis added a comment -

          Per the discussion on CASSANDRA-5043, add doesn't need to support out-of-order; only replace does.

          Show
          Jonathan Ellis added a comment - Per the discussion on CASSANDRA-5043 , add doesn't need to support out-of-order; only replace does.
          Hide
          Jonathan Ellis added a comment -

          I note that the reason we need to support add-not-in-sorted-order and replace is purely counters. CASSANDRA-4775 could fix that.

          Show
          Jonathan Ellis added a comment - I note that the reason we need to support add-not-in-sorted-order and replace is purely counters. CASSANDRA-4775 could fix that.
          Hide
          Jonathan Ellis added a comment - - edited

          A Flyweight pattern is one approach. Something like

          class FlyweightColumns
          {
            ByteBuffer[] names;
            ByteBuffer[] values;
            long[] timestamps;
          
            private class FlyweightColumn implements IColumn
            {
              private final int index;
          
              public ByteBuffer name()
              {
                return names[index];
              }
          
              ... 
            }
          }
          
          Show
          Jonathan Ellis added a comment - - edited A Flyweight pattern is one approach. Something like class FlyweightColumns { ByteBuffer[] names; ByteBuffer[] values; long [] timestamps; private class FlyweightColumn implements IColumn { private final int index; public ByteBuffer name() { return names[index]; } ... } }

            People

            • Assignee:
              Unassigned
              Reporter:
              Jonathan Ellis
            • Votes:
              1 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development