the locking in MultiFn.java (synchronized methods) can cause lots of contention in multithreaded programs

Description

if you call a single multimethod a lot in multithreaded code you get lots of contention for the lock on the multimethod. this contention slows things down a lot.

this is due to getMethod being synchronized. it would be great if there was some fast non-locking path through the multimethod.

Environment

None

Attachments

3

Activity

Show:

Rich Hickey September 17, 2012 at 3:20 PM

tests-only patch ok

Andy Fingerhut September 15, 2012 at 4:59 PM

Patch clj-988-tests-only-patch-v1.txt dated Sep 15 2012 is a subset of David Santiago's
patch issue988-lockless-multifn+tests-120817.diff dated Aug 17 2012. It includes only the tests from that patch. Applies cleanly and passes tests with latest master after Rich's read/write lock change for multimethods was committed.

Rich Hickey September 15, 2012 at 3:05 PM

https://github.com/clojure/clojure/commit/83ebf814d5d6663c49c1b2d0d076b57638bff673 should fix these issues. The patch here was too much of a change to properly vet.

If you could though, I'd appreciate a patch with just the multimethod tests.

David Santiago August 18, 2012 at 1:49 AM

As promised, I reimplemented those two functions. I also added more multimethod tests to the test suite. The new tests should definitely prevent a similar mistake. While I was at it, I went through core.clj and found all the multimethod API functions I could and ensured that there were at least some basic functionality tests for all of them. The ones I found were: defmulti, defmethod, remove-all-methods, remove-method, prefer-method, methods, get-method, prefers (Some of those already had tests from the earlier version of the patch).

Really sorry about catching this right after you vetted the patch. 12771 test assertions were apparently not affected by prefers and methods ceasing to function, but now there are 12780 to help prevent a similar error. Since you just went through it, I'm leaving the older version of the patch up so you can easily see the difference to what I've added.

David Santiago August 18, 2012 at 1:44 AM

Latest patch for this issue. Supersedes issue988-lockless-multifn+tests.diff as of 08/17/2012.

Completed

Details

Assignee

Reporter

Approval

Ok

Patch

Code and Test

Priority

Fix versions

Created May 8, 2012 at 5:14 PM
Updated September 21, 2012 at 8:21 PM
Resolved September 21, 2012 at 8:21 PM