-
-
Notifications
You must be signed in to change notification settings - Fork 23
Better Support for new MM #17
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
Better Support for new MM #17
Conversation
adea4a7
to
9a12432
Compare
@ychescale9 this is now ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@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.native
level 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.