[SPARK-18196] [CORE] Optimise CompactBuffer implementation #15713
Conversation
See the JIRA for details - summary is slightly increased footprint in the class but 4% performance increase observed on PageRank with HiBench large
|
It leads to a bigger memory footprint because of the extra elements that may be allocated but unused in the array? and the performance win is probably avoiding those branches? I could believe it. But then there's little point to this class. Reading the original problem statement, I wonder why it wasn't easier to just allocate an ArrayBuffer with a small initial capacity? |
|
Test build #67898 has finished for PR 15713 at commit
|
|
+CC @rxin who can elaborate better. For specific workloads, ofcourse this might not be the case. Having said that, this was done in 1.0-1.1 timeframe, and perhaps not applicable anymore ? |
|
Thanks for the prompt feedback, we found this opportunity when profiling Spark 1.6.2 with HiBench large and again this showed up as a hot method with the PageRank benchmark, we can gather data to see if it's still hot with Spark 2 also and I'm planning to contribute lots of similar improvements Paraphrasing from a colleague:
Note that with our testing using OpenJDK 8 we didn't see a noticeable performance improvement (nor a regression) despite the very slight footprint increase (an increase of 2 MB instead of 1.5 MB), ideally we'll improve the performance for everybody so there may be scope for optimisations here that'll be of use to OpenJDK users too |
|
I believe the benchmark, in which case it seems like this class could just be removed at this point, and replaced with an ArrayBuffer that sets an appropriately small initial size. Is that possible to easily benchmark too while you have the machinery in place? |
|
Good point, I'll first see if we can build and pass the unit tests by replacing CompactBuffer with ArrayBuffer[2] and we'd also repeat the profiling (this would verify if groupByKey is still hot for PageRank). I'll keep the CompactBufferSuite for our experiment and replace the CompactBuffer references with ArrayBuffer to verify we can still perform all of the operations that the tests exercise. My concern is that in comment for the CompactBuffer class we see the downsides of said ArrayBuffer (the 16 entry Object array we'd get, so we'd indeed want a default two element ArrayBuffer that only grows if needed -- which sounds like what we currently have but encapsulated within this class so we can override two operators and grow the backing array as required) |
|
Having worked with @a-roberts to help develop this change I can see that the type itself may not be required any more with the change proposed, but keeping it around would allow us to distinguish between ArrayBuffers which are supposed to be small and ArrayBuffers that are general purpose so that if the best trade-off for the case of small buffers changes again in the future we still have the single point at which to tune it. I would be interested to hear @srowen thoughts on future tuneability vs maintenance. |
|
@andrewcraik If we are simply delegating to ArrayBuffer, it makes sense to remove the class - it is an additional object created for each key being aggregated. Having said that, I do think the original intention for the class continues to hold - depends on cardinality of values per key. For larger cardinality, it might be as simple changing order of conditions to improve performance: newIndex > 1 followed by 0, 1. |
|
I do agree we should save the extra object allocation, but again whether we should flatten the array into the class or remove the class and use ArrayBuffer directly is the question - do we want to preserve the semantic distinction of the type intents or loose it for simplicity? I'm not sure what the right trade-off is, but preserving the class would provide future ability to tune that would be lost by removal of the type. While reordering branches might help a bit, the size of the method in bytecodes is inflated by having the primitive fields for 0 and 1 leading to reduced icache locality, longer distance branches and more complexity for the JIT compiler to handle. Part of the performance gain from removing the field is just from making the code smaller and simpler for the Java JIT. Perhaps if the feeling is that there is value in the semantic meaning of the type due to the trade-offs it implies finding some way to not just delegate might be the best way forward? |
|
What is this benchmark? Can you show the query and the explain plan, if it is in Spark SQL? The current change doesn't make a lot of sense. If the findings are correct, we should just use ArrayBuffer (or java ArrayList) directly. |
|
The benchmark used is HiBench 5 which works with Spark 1.6, I expect there will be HiBench 6 officially available soon with Spark 2 support We can see here that the Spark PageRank example is used and that the main functions, in order of their occurrence, are textFile, map, split, slice, groupByKey, mapValues, join, flatMap, reduceByKey |
|
The benchmark sounds fine. I think it would indeed be interesting to see how a simple ArrayBuffer works, then. |
|
So it seems like the benchmark is the worst case for the current implementation -- it's doing a big groupByKey to potentially add a lot of items to the array buffer. Do we have cases that only add one or two items? |
|
@a-roberts I'm interested in your thoughts on that last point which is a fair one. If it's as-good for small cases and better for large cases to just use ArrayBuffer, that's a great win. |
|
I'll add a print to keep track of how big the CompactBuffer actually gets when used with, say, SparkSqlPerf (1 scale factor then 2 scale factor) and HiBench. Once this is complete and we have an idea of how big it gets for these workloads, I'll be running the unit tests with an ArrayBuffer(2) replacing all CompactBuffer instances, I have the code ready and hoping to soon finish up the Partitioned* class PR to speed up the PageRank implementation and to contribute the first phase of improvements to the SizeEstimator. |
|
In response to @rxin's question, for HiBench CompactBuffers are used only on PageRank (none of the other 11) and these buffers mainly have between 3 and 40 elements, no more than 60, never with only two elements. The PageRank workload processes 500k pages (large profile), we have 500k CompactBuffer constructor calls and 500k prints in the += method when curSize <= 2, indicating they're always expanding. I don't know of any cases where we're adding only a couple of elements, I also ran SparkSqlPerf, all 100 queries, again we have no output indicating that we use this class (no prints from the constructor, the growToSize or the += methods). Here's a breakdown of growBySize invocations (prints the curSize variable) with PageRank so we have an idea of how big the CompactBuffers actually become. I used the Spark WordCount example on the 677mb stdout file containing my prints to generate this data and we have a total of 18,762,361 growth events. On the left we have the CompactBuffer size in elements and on the right we have a number representing how many times this appeared in the output file (therefore the CompactBuffer has grown to have this many elements that many times). If there are better ways to figure this out or other workloads to suggest do let me know, I've got the code ready that replaces CompactBuffer with ArrayBuffer(2) for profiling and testing. |
|
Performance results against the Spark master branch on a 48 core machine running PageRank with 500k pages follow Vanilla CompactBuffer, no changes, run time and throughput (bytes per second) provided The commit here Way too similar so attributing this to benchmark noise, without the 51s run this would be a few percentage points worse though Use an ArrayBuffer (initial capacity of 16, default) instead of CompactBuffer Use an ArrayBuffer (initial capacity of 2) instead of CompactBuffer With my tests I see that using an ArrayBuffer is noticeably worse, so I'll continue to look into what's going on to see if we can improve performance here as this is definitely a hot codepath for this particular algorithm |
|
@a-roberts feel free to continue investigating, but at some point it's also important to ask the question: is this micro-optimization worth it? It only improves 4% for a very specific workload based on your measurement, and might regress other workloads but we are not sure about that. It also takes time from a lot of people to look at ... |
|
Although I'd love to find we can get rid of a custom collections class, it seems like we can't do that and that the optimization here doesn't result in a win. Let's close this for now. |
Closes apache#12968 Closes apache#16215 Closes apache#16212 Closes apache#16086 Closes apache#15713 Closes apache#16413 Closes apache#16396
What changes were proposed in this pull request?
See the JIRA for details - summary is slightly increased footprint in the class but 4% performance increase observed on PageRank with HiBench large
How was this patch tested?
Existing unit tests and HiBench large