Will it be less of a problem if the class used simple synchronized block instead of a ReentrantLock? It looks like it can be done with synchronized blocks.
This is how it worked prior to
HADOOP-3617. ReentrantLock is supposed to be a more lightweight locking mechanism, but in MapOutputBuffer there is very little contention for the lock, so it's possible that "heavier" mechanisms may actually take less time. I wouldn't expect there to be a dramatic difference in execution time, though. The principal cost is inherent in the design, which requires coordination between the writer and the spill thread. Given the structure of the serialization, the MapOutputBuffer requests the given record to serialize itself into an internal buffer, which it effects through a series of writes back into the data structure. Since the record size is unknown a priori, the writer must obtain a lock on the structure because it's possible that the in-memory structure must be spilled to accommodate the writes composing the record, that it must wait for the spill to complete, etc.
There are also some subtle points when starting/completing the spill thread that are also more easily expressed in ReentrantLocks.
I understand why we should try to avoid extra buffers in general, but is this a place where we could introduce a configurable buffer? Maybe if the count is set to 1, we skip the buffer, otherwise push to the spill thread only every N writes?
How would we size this buffer, unless the user promises to give us fixed-size records? We could allocate extents for MapOutputBuffer and use queues to push them between the collection and spill threads. That might actually be a clearer way to express some of the logic in MapTask, though the serialization becomes slightly more complex and we'd add a layer of indirection.