[ https://issues.apache.org/jira/browse/CASSANDRA-15389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17015387#comment-17015387 ]
Benedict Elliott Smith edited comment on CASSANDRA-15389 at 1/14/20 9:01 PM: ----------------------------------------------------------------------------- Great! A couple of really minor things: * I still see the Long.MIN_VALUE sentinel, maybe you forgot to push latest commit? * I don't think any caller of {{apply}} uses {{stopCondition}} anymore? * In both {{apply}} and {{accumulate}} we _could_ (at your discretion) ** Test {{<= childOffset}} to avoid the slightly confusing maths to calculate {{limit}} ** Keep the guard as {{< childOffset}} and just repeat the child logic, since it's only a single line in each case. I find it makes it clearer that we're visiting one more child than we are keys, and it gives the branch predictor less to do (though there's a good chance the compiler will do it anyway, if we use {{<= childOffset}} in the guard), but that's just my way of thinking not any objective fact Otherwise looks ready to me was (Author: benedict): Great! A couple of really minor things: * I still see the Long.MIN_VALUE sentinel, maybe you forgot to push latest commit? * I don't think any caller of {{apply}} uses {{stopCondition}} anymore? * In both {{apply}} and {{accumulate}}: ** We could test {{<= childOffset}} to avoid the slightly confusing maths to calculate {{limit}} ** I have a very mild preference for keeping it to {{< childOffset}} and just repeating the child logic, particularly since it's only a single line in each case, as I find it makes it clearer we're visiting one more child, and it gives the branch predictor less to do (though there's a good chance the compiler will do it anyway, if we use {{<= childOffset}} in the guard) Otherwise looks ready to me > Minimize BTree iterator allocations > ----------------------------------- > > Key: CASSANDRA-15389 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15389 > Project: Cassandra > Issue Type: Sub-task > Components: Local/Compaction > Reporter: Blake Eggleston > Assignee: Blake Eggleston > Priority: Normal > Fix For: 4.0 > > > Allocations of BTree iterators contribute a lot amount of garbage to the > compaction and read paths. > This patch removes most btree iterator allocations on hot paths by: > • using Row#apply where appropriate on frequently called methods > (Row#digest, Row#validateData > • adding BTree accumulate method. Like the apply method, this method walks > the btree with a function that takes and returns a long argument, this > eliminates iterator allocations without adding helper object allocations > (BTreeRow#hasComplex, BTreeRow#hasInvalidDeletions, BTreeRow#dataSize, > BTreeRow#unsharedHeapSizeExcludingData, Rows#collectStats, > UnfilteredSerializer#serializedRowBodySize) as well as eliminating the > allocation of helper objects in places where apply was used previously^[1]^. > • Create map of columns in SerializationHeader, this lets us avoid > allocating a btree search iterator for each row we serialize. > These optimizations reduce garbage created during compaction by up to 13.5% > > [1] the memory test does measure memory allocated by lambdas capturing objects -- This message was sent by Atlassian Jira (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org