Welcome! Please see the About page for a little more info on how this works.

+6 votes
ago in core.async by

Hi! While reading the channel implementation I noticed the internal queue is a java.util.LinkedList:
https://github.com/clojure/core.async/blob/master/src/main/clojure/clojure/core/async/impl/channels.clj

The common JVM guidance these days is to prefer java.util.ArrayDeque for FIFO/LIFO queues due to better locality and lower GC overhead. For example, the JDK docs state: “This class is likely to be faster than Stack when used as a stack, and faster than LinkedList when used as a queue.”
https://docs.oracle.com/javase/8/docs/api/java/util/ArrayDeque.html
Related SO discussion: https://stackoverflow.com/questions/6163166/why-is-arraydeque-better-than-linkedlist

A few questions for the maintainers:

  1. Was LinkedList originally chosen for a specific reason (e.g., very old JDK compatibility)?
  2. Are there behaviors in channels that specifically rely on LinkedList (e.g., allowing null, I believe channels disallow nil anyway, or particular iterator characteristics), or would ArrayDeque be a drop-in replacement for the add/remove-from-ends operations used?
  3. If we provide a small PR and benchmark showing an improvement (lower allocations / better throughput under contention), would such a change be considered?
  4. Since ArrayDeque is available on Java 8+, and current Clojure/core.async baselines target that or newer, is there any remaining compatibility concern?

Context: there was an older conversation in Clojurians (#clojure) that suggested ArrayDeque could reduce GC pressure under load and that the original choice may have been influenced by older JDKs:
https://clojurians.slack.com/archives/C03S1KBA2/p1526551888000376

If this sounds reasonable, I’m happy to run the core.async test suite, add JMH-style benchmarks focused on channel put/take hot paths, and submit a PR.

Thanks!

1 Answer

+1 vote
ago by

There is a ticket for this at https://clojure.atlassian.net/browse/ASYNC-231

ago by
Oh nice, thanks.
I don't have permissions to comment there, but I have some reservations about the patch's contents,  specifically with regard to the use of `(count this)` instead of `(.size buf)`.  
Going through the protocol's `count` will needless box the `long`.
...