Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Oct 26, 2021
Merged

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

merged 10 commits into from
Oct 26, 2021

Conversation

josh-7t
Copy link

@josh-7t josh-7t commented Oct 5, 2021

See #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.

Copy link
Collaborator

@KingMob KingMob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together.

  1. core.memoize is fine, but mostly for being more thread-safe. I don't think we ever care to evict in this case, so we probably don't need to consider the various eviction strategies.
  2. Likewise, the thread "unsafe" aspect of plain memoize is that it might duplicate work, which is fine if it's still sufficiently faster than the alternatives
  3. The test fn sleeps for 100 ms. If these results are in nanoseconds, that means sleep time dominates over cache management by orders of magnitude, which is going to make all the memoization methods perform about the same when it comes to the first time. To evaluate initial memoization performance, I don't think we really need a time-consuming fn.
  4. Minor note: I'm surprised that memo-memo is frequently slower than memo-fifo.

So far, it looks like plain memoize may be fastest. I'm surprised that fast-memoize is 100x slower than it, though. Either it's truly from a different era (i.e., 8 years ago) or something's wrong.

(EDIT: removed the bit about pmap. After getting some sleep, it struck me that of course we want to test under parallel scenarios, even if it's more convoluted. )

@KingMob KingMob self-assigned this Oct 9, 2021
@KingMob
Copy link
Collaborator

KingMob commented Oct 9, 2021

@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.

@KingMob KingMob marked this pull request as ready for review October 9, 2021 19:16
@josh-7t
Copy link
Author

josh-7t commented Oct 13, 2021

@josh-7t
Copy link
Author

josh-7t commented Oct 21, 2021

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

@KingMob
Copy link
Collaborator

KingMob commented Oct 26, 2021

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

@KingMob KingMob merged commit b89e027 into clj-commons:master Oct 26, 2021
@josh-7t
Copy link
Author

josh-7t commented Oct 27, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants