Better Support for new MM#17
Conversation
adea4a7 to
9a12432
Compare
|
@ychescale9 this is now ready for review! |
ychescale9
left a comment
There was a problem hiding this comment.
Great work! Just left a couple of comments 😃
| reading.set( | ||
| reading.get().let { currentReading -> | ||
| reading.value = | ||
| reading.value.let { currentReading -> |
There was a problem hiding this comment.
Should we use update(function: (T) -> T) instead?
It would also address this from the atomicfu dos and don'ts:
Do not introduce complex data flow in parameters to atomic variable operations, i.e. top.value = complex_expression and top.compareAndSet(cur, complex_expression) are not supported (more specifically, complex_expression should not have branches in its compiled representation). Extract complex_expression into a variable when needed.
| cacheEntry.accessTimeMark.value = (accessTimeMark + accessTimeMark.elapsedNow()) | ||
| } | ||
| if (expiresAfterWrite) { | ||
| val writeTimeMark = cacheEntry.writeTimeMark.value | ||
| cacheEntry.writeTimeMark.set(writeTimeMark + writeTimeMark.elapsedNow()) | ||
| cacheEntry.writeTimeMark.value = (writeTimeMark + writeTimeMark.elapsedNow()) |
There was a problem hiding this comment.
these aren't complex expression but perhaps we can also use update(function: (T) -> T) just in case.
|
@ychescale9 addressed PR comments! |
|
If this gets merged it'll be interesting to see if #9 is still an issue. |
38b7741 to
0bcaac6
Compare
@dcvz do you have any insights on why sharing between threads is no longer an issue? AFAICT the implementation is no longer thread-safe as |
@ychescale9 The
I think that's a different concern.. You're talking about having a "ConcurrentMap" -- which would make concurrent access safe. There's some implementations out there of one, but I don't think we necessarily need one. Currently the only thing we'd need to make concurrently safe is the |
|
My original jvm only implementation used The AndroidX collection library is being migrated to KMP https://android-review.googlesource.com/c/platform/frameworks/support/+/2070948 and it's using a lock to guard all operations. It's probably not as efficient as |
Since we are using the new KMM memory model - this PR improves things:
Addresses #16
Note:
I attempted to keep support for both memory models, but the flag is only available at the
kotlin.nativelevel and supporting it would've created a lot of boilerplate code in the project just to expose the flag to common.Ktor in their latest release dropped support for the old memory model, so I think it's a safe option to as well.