search

JakeWharton / DiskLruCache

Java implementation of a Disk-based LRU cache which specifically targets Android compatibility.
http://jakewharton.github.io/DiskLruCache
Apache License 2.0
5.79k stars 1.18k forks source link

Replaced synchronize's with ReentrantReadWriteLock & added contains() method #56

Closed Wavesonics closed 10 years ago

Wavesonics commented 10 years ago

This PR removes all synchronized statements in favor of a ReentrantReadWriteLock. This allows read operations to not block each other, but write operations to block everything (the way synchronized was working previously).

With the addition of a simple contains() method, it is now super cheap to check if an image exists in the cache.

Wavesonics commented 10 years ago

This is a 2nd attempt at addressing #54

gubatron commented 10 years ago

Just read your code, read the other posts related to the motivation and I like the initiaive. Everything seems good but honestly I think we can do better without any locks at all as you're still at the mercy of deadlocks.

Why not fix all the way by making it a Lock-Free data structure using Atomic instructions instead (java.util.concurrent.atomic.*), let's use the hardware instructions made to solve these issues and achieve a greater level of parallelism and performance.

just saying ;)

Wavesonics commented 10 years ago

You're probably right. This is currently failing one of the unit testing on Travis which I'm looking into now.

However, the read write lock implementation is still better that the synchronized implementation I believe. So maybe it would be worth merging it until a better implementation using atomics is devised? On Feb 10, 2014 1:51 PM, "Angel Leon" notifications@github.com wrote:

Just read your code, read the other posts related to the motivation and I like the initiaive. Everything seems good but honestly I think we can do better without locks at all as you're still at the mercy of deadlocks.

Why not fix all the way by making it a Lock-Free data structure using Atomic instructions instead (java.util.concurrent.atomic.*), let's use the hardware instructions made to solve these issues and achieve a greater level of parallelism and performance.

just saying ;)

Reply to this email directly or view it on GitHubhttps://github.com/JakeWharton/DiskLruCache/pull/56#issuecomment-34687021 .

gubatron commented 10 years ago

can you write a simple benchmark test to prove this performs better?

Wavesonics commented 10 years ago

Yes, I will also write a unit test for the new contains() method. On Feb 10, 2014 2:02 PM, "Angel Leon" notifications@github.com wrote:

can you write a simple benchmark test to prove this performs better?

Reply to this email directly or view it on GitHubhttps://github.com/JakeWharton/DiskLruCache/pull/56#issuecomment-34688035 .

Wavesonics commented 10 years ago

Other people who have forked from this master have also seen this test failure after not changing anything of substance.

The method that is causing the assert also states that it "Returns the approximate total number of tasks that have ever been scheduled for execution"

It looks to me like the test is meant to prove that only 1 evict was caused by the shrink, is there a better way we can test this?

Wavesonics commented 10 years ago

Closing this, there is another PR #59 that is from it's own branch, and contains unit tests for the new contains method.