Replace Clojure functions that contain metadata with custom FnWithMeta

[alexander-yakushev]

As @bshepherdson mentioned in #149, a lot of the overhead that Methodical brings comes from packing/unpacking arglists in vararg functions, and that in turn is primarily caused by with-meta on functions creating very inefficient RestFn objects.

When I say "overhead", I'm talking about 300 ns and ~500 bytes allocated per call of a 2-argument Methodical multimethod, vs 15ns and 0b when called an identical Clojure multimethod. The allocation part is especially damning as it creates memory and GC pressure and prevents JIT from optimizing nested fns (and Methodical likes nesting lambdas a lot).

I've made several attempts to address this:

• Disable attaching metadata to methods. Obviously, that didn't work, as Methodical relies on method metadata for its proper operation. At first, I thought metadata was only for debugging, proved to be wrong.
• Store all methods as wrapper deftype (fn+meta), and then extract the fn each time the method is to be called. In the end, I've spent too much time adapting all the tests to this new paradigm, and god knows how much I had left to rewrite.
• Introduce a custom class FnWithMeta which extends RestFn and proxies all invoke() calls to the stored function and, obviously, stores metadata in a field.

This PR contains my last attempt. All tests pass and everything seems to be working. The performance of 2-arity method call is back to 12-15ns and 0b (so it's even somewhat faster than Clojure multimethods). This is a screenshot of the profile:

There are two drawbacks to this solution:

• Methods are stored as wrappers, even if the wrappers behave the same as the wrapped functions. You now have to be careful about comparing the stored method to the method passed to add-primary-method etc. I had to adapt a few tests for this.
• The introduced class is a Java class which complicates the build process a bit. You now have to call clojure -T:build javac before running tests or starting the REPL.

Eventually, #149 or a variation of it will have to be merged too in order to ensure wrapping/unwrapping of arglists doesn't happen in 4,5,6-argument multimethods which are quite popular in Toucan, for example.

---

• Ensure the PR follows the Clojure Style Guide.
• New features are documented, or documentation is updated appropriately for any changed features.
• Carefully review your own changes and revert any superfluous ones. (A good example would be moving words in the

[alexander-yakushev]

@bshepherdson I've taken your commit from #149 and sorted out the tests for it to pass and then applied my changes on top. Together, this achieves good improvements in Metabase, the overhead from Methodical is mostly gone.

[camsaul]

I'm really wary of adding a Java class here since adding ClojureScript support to Methodical is definitely something I want to pursue at some point in the future. Couldn't you just create a custom deftype that implements clojure.lang.IFn and clojure.lang.IObject?

[alexander-yakushev]

The problem is RestFn does more than that. It also implements a bunch of doInvokes and helpers to make apply less expensive if the number of required arguments for the function is bigger than the number of supplied values. Rewriting that full class to a deftype would be daunting to say the least.

When Clojurescript support becomes desirable, you should be able to refactor the new functions (fn-with-meta, vary-fn-with-meta, unwrap) to defer to the plain with-meta for fn objects in ClojureScript, using reader conditionals.

[alexander-yakushev]

And proxy is very bad performance-wise.

[camsaul]

Would it really be that much more complicated than the other places we're already doing it, like here?

methodical/src/methodical/impl/standard.clj

Lines 187 to 233 in 274d0ef

	clojure.lang.IFn
	(invoke [_]
	(invoke-multifn impl mta))
	(invoke [_ a]
	(invoke-multifn impl mta a))
	(invoke [_ a b]
	(invoke-multifn impl mta a b))
	(invoke [_ a b c]
	(invoke-multifn impl mta a b c))
	(invoke [_ a b c d]
	(invoke-multifn impl mta a b c d))
	(invoke [_ a b c d e]
	(invoke-multifn impl mta a b c d e))
	(invoke [_ a b c d e f]
	(invoke-multifn impl mta a b c d e f))
	(invoke [_ a b c d e f g]
	(invoke-multifn impl mta a b c d e f g))
	(invoke [_ a b c d e f g h]
	(invoke-multifn impl mta a b c d e f g h))
	(invoke [_ a b c d e f g h i]
	(invoke-multifn impl mta a b c d e f g h i))
	(invoke [_ a b c d e f g h i j]
	(invoke-multifn impl mta a b c d e f g h i j))
	(invoke [_ a b c d e f g h i j k]
	(invoke-multifn impl mta a b c d e f g h i j k))
	(invoke [_ a b c d e f g h i j k l]
	(invoke-multifn impl mta a b c d e f g h i j k l))
	(invoke [_ a b c d e f g h i j k l m]
	(invoke-multifn impl mta a b c d e f g h i j k l m))
	(invoke [_ a b c d e f g h i j k l m n]
	(invoke-multifn impl mta a b c d e f g h i j k l m n))
	(invoke [_ a b c d e f g h i j k l m n o]
	(invoke-multifn impl mta a b c d e f g h i j k l m n o))
	(invoke [_ a b c d e f g h i j k l m n o p]
	(invoke-multifn impl mta a b c d e f g h i j k l m n o p))
	(invoke [_ a b c d e f g h i j k l m n o p q]
	(invoke-multifn impl mta a b c d e f g h i j k l m n o p q))
	(invoke [_ a b c d e f g h i j k l m n o p q r]
	(invoke-multifn impl mta a b c d e f g h i j k l m n o p q r))
	(invoke [_ a b c d e f g h i j k l m n o p q r s]
	(invoke-multifn impl mta a b c d e f g h i j k l m n o p q r s))
	(invoke [_ a b c d e f g h i j k l m n o p q r s t]
	(invoke-multifn impl mta a b c d e f g h i j k l m n o p q r s t))
	(invoke [_ a b c d e f g h i j k l m n o p q r s t args]
	(apply invoke-multifn impl mta a b c d e f g h i j k l m n o p q r s t args))
	(applyTo [_ args]
	(apply invoke-multifn impl mta args))

[alexander-yakushev]

That class is 3k lines of code. I guess with macros it can be mostly generated. I can try to reverse engineer the full details of what RestFn is doing and write such a macro.

[alexander-yakushev]

I noticed only now that I extend AFunction, not RestFn. So, perhaps, it is indeed doable.

[alexander-yakushev]

Turned out it wasn't as much work as I thought it was. I've updated the PR, no more Java classes now.

[camsaul]

It would be nice to add a Kondo :discouraged-var rule for clojure.core/partial but I can do that separately

[camsaul]

I wonder if maybe we should also have a with-meta* that does the right thing for functions but otherwise does normal with-meta and then have Kondo :discouraged-var tell everyone to use that. Then we don't need fn-with-meta and it's impossible to forget to use it

[alexander-yakushev]

Good idea!

[camsaul]

Suggested change

#_(println "invoke-multifn 5-arity")