Skip to content

Conversation

NHDaly
Copy link
Collaborator

@NHDaly NHDaly commented Feb 15, 2022

  • Fix thread-safety violation by accidentally not locking the base cache during delete!
  • Fix concurrency-safety by not holding the get!() on the thread-local cache across the whole execution
  • Use per-thread locks to make invariant to Task migration. ...... In retrospect, this does make this whole structure start to look a lot like a Dict + a Read/Write lock, and I wonder how their performance would compare......

Actually, this didn't seem to have too much perf impact, which is nice:

┌ Info: benchmark results
│   Threads.nthreads() = 1
│   time_serial = 0.03260747
│   time_parallel = 0.152217246
└   time_baseline = 0.131397639
┌ Info: benchmark results
│   Threads.nthreads() = 20
│   time_serial = 0.030174521
│   time_parallel = 0.307062643
└   time_baseline = 1.239406026
┌ Info: benchmark results
│   Threads.nthreads() = 100
│   time_serial = 0.030373533
│   time_parallel = 1.771401144
└   time_baseline = 5.397114559

@NHDaly NHDaly force-pushed the nhd-concurrency-safety-fixes branch from 8772cc7 to c6f8a69 Compare February 15, 2022 18:58
...... In retrospect, this does make this whole structure start to look
a lot like a Dict + a Read/Write lock, and I wonder how their
performance would compare......
@NHDaly NHDaly force-pushed the nhd-concurrency-safety-fixes branch from c6f8a69 to 76a48d5 Compare February 15, 2022 18:59

const CACHE_MISS = :__MultiThreadedCaches_key_not_found__

function Base.get!(func::Base.Callable, cache::MultiThreadedCache{K,V}, key) where {K,V}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh the indentation change here down is annoying. I'd recommend hiding whitespace in the github UI:

Screen Shot 2022-02-15 at 6 03 11 PM

@NHDaly NHDaly merged commit 24cb6f2 into nhd-initial-code Feb 17, 2022
@NHDaly NHDaly deleted the nhd-concurrency-safety-fixes branch February 17, 2022 05:01
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.

1 participant