Skip to content
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

Rewrite the implementation of the linked list for JobSupport #4095

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

dkhalanskyjb
Copy link
Contributor

Fixes #3886

One of the steps for #3887

A draft because

  • it's undertested
  • and underdocumented,
  • it approximately doubles the memory consumption of JobSupport.

@dkhalanskyjb
Copy link
Contributor Author

@qwwdfsad, could you help with optimizing the memory consumption? Not requesting a proper review yet because I know already the code is messy, I just expect it to get even messier during memory consumption optimization.

Base automatically changed from dk-remove-dcss-2 to develop May 21, 2024 12:54
With this change, `JobSupport` uses only a small and well-defined
portion of the functionality `LockFreeLinkedList` provides, which
makes it easier to replace the list implementation.
In exchange, now, removal is linear in the size of number of
registered handlers instead of being amortized constant.
@dkhalanskyjb
Copy link
Contributor Author

For some reason, tests hang after a rebase, exclusively on Wasm/JS.

@dkhalanskyjb
Copy link
Contributor Author

After some discussions with @igoriakovlev, we pinpointed this to the following behavior that occurs on Wasm/JS with Kotlin 2.0.0 but not with other platforms or Wasm/JS with Kotlin 1.9.24: https://pl.kotl.in/OtO03tnT-
The issue occurs on

if (cells[indexInSegment].compareAndSet(value, node)) return true
Here, CAS never succeeds on Wasm/JS, since identity comparisons on value classes are prohibited and may return undefined results. The straightforward "fix" is to make BrokenForSomeElements a non-value class, but we're currently negotiating about having value classes work with CAS consistently.

This shouldn't block further work on this PR, we can work around this at the last moment if needed.

@dkhalanskyjb
Copy link
Contributor Author

dkhalanskyjb commented May 23, 2024

It's not just value classes; apparently, all types that can be boxed aren't guaranteed to do something sensible with === in multiplatform Kotlin, including primitive types.

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.

None yet

1 participant