search

clj-commons / byte-streams

A Rosetta stone for JVM byte representations
417 stars 33 forks source link

use memoize to remove runtime error in graal native image #50

Closed josh-7t closed 2 years ago

josh-7t commented 2 years ago

See https://github.com/clj-commons/byte-streams/pull/48 for the error encountered in graal.

According to the simple benchmarks that I came up with it looks like memoize and fast-memoize are very close in performance.

I also benchmarked the different options available in https://github.com/clojure/core.memoize in case one of the caching strategies there is useful.

KingMob commented 2 years ago

@josh-7t Well, I've been playing around with it for the last hour, using Criterium for a second opinion (because Criterium factors in its own overhead), and the results are the same.

I'm not sure why the ConcurrentHashMap-backed version is 1000x slower, but my guess is it's related to the wrapping/unwrapping of the value in a delay, or maybe the bookkeeping overhead in ConcurrentHashMap. I was worried the delay was necessary for certain behavior, but it's unwrapped immediately. It replaced some older code that swapped nils in and out with ::nil because ConcurrentHashMa can't store nulls, so I don't think it's relevant.

I don't think it's worth investigating more deeply, so let's go with plain memoize. Can you clean up? Let's remove the core.memoize and metrics-clojure deps, and move byte-streams-memo-benchmark into a gist in case we ever need to revisit it.

josh-7t commented 2 years ago

Gist created: https://gist.github.com/josh-7t/3acdfe6333f67bdd9355afb4dfd6221c

josh-7t commented 2 years ago

Just checking in, I've made the requested changes. Is this good to go in?

KingMob commented 2 years ago

Sorry, I moved to another country, and have been a bit distracted. Merging now.

josh-7t commented 2 years ago

Hey, no worries. That's a big thing! Thanks very much!