Skip to content

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

Merged
merged 7 commits into from
Apr 16, 2022

Conversation

dcvz
Copy link
Contributor

@dcvz dcvz commented Apr 15, 2022

Since we are using the new KMM memory model - this PR improves things:

  • moves away from IsoMutableMap (as sharing between threads is no longer an issue)
  • replaces stately (only used for atomics/locks now) with atomic-fu (made by kotlin team)

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.

@dcvz dcvz force-pushed the feature/kmm-new-mm branch from adea4a7 to 9a12432 Compare April 15, 2022 17:06
@dcvz dcvz marked this pull request as ready for review April 16, 2022 00:03
@dcvz dcvz requested a review from ychescale9 as a code owner April 16, 2022 00:03
@dcvz
Copy link
Contributor Author

dcvz commented Apr 16, 2022

@ychescale9 this is now ready for review!

Copy link
Member

@ychescale9 ychescale9 left a 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 😃

Comment on lines 31 to 33
reading.set(
reading.get().let { currentReading ->
reading.value =
reading.value.let { currentReading ->
Copy link
Member

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.

Comment on lines 231 to 235
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())
Copy link
Member

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.

@dcvz
Copy link
Contributor Author

dcvz commented Apr 16, 2022

@ychescale9 addressed PR comments!

@dcvz
Copy link
Contributor Author

dcvz commented Apr 16, 2022

If this gets merged it'll be interesting to see if #9 is still an issue.

@dcvz dcvz force-pushed the feature/kmm-new-mm branch from 38b7741 to 0bcaac6 Compare April 16, 2022 12:52
Copy link
Member

@ychescale9 ychescale9 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ychescale9 ychescale9 merged commit a1d460a into ReactiveCircus:main Apr 16, 2022
@ychescale9
Copy link
Member

moves away from IsoMutableMap (as sharing between threads is no longer an issue)

@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 MutableMap isn't thread-safe and we're not putting all the operations behind a reentrantLock.
Wonder if we should revert back to Stately's IsoMutableMap for now?

@dcvz
Copy link
Contributor Author

dcvz commented Apr 22, 2022

do you have any insights on why sharing between threads is no longer an issue? AFAICT the implementation is no longer thread-safe as MutableMap isn't thread-safe

@ychescale9 The IsoMutableMap put the map behind an isolated state, so you could use it from any thread without running into the common freezing issues in iOS -- but it made access expensive. In the new memory model there's no longer freezing and no requirement that mutable state must always be accessed from the same thread.

and we're not putting all the operations behind a reentrantLock

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 get(loader:) given that we don't want two concurrent operations to make the possibly expensive operation of loading (we already do this).. But the normal get and put do not suffer from this.. you might get right as you put and get null since the value is not set yet, but I think that should be fine.

@ychescale9
Copy link
Member

My original jvm only implementation used ConcurrentHashMap which allowed thread-safe concurrent access so I thought this might be a bit of downgrade for jvm users.

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 ConcurrentHashMap but could be a quick win to support thread safe concurrent access until there's a good multiplatform thread-safe concurrent map implementation.

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