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
Collaborator

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 dkhalanskyjb force-pushed the dk-remove-linked-list branch from 176b417 to 9b9148c Compare April 11, 2024 12:44
@dkhalanskyjb
Copy link
Collaborator 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.

@dkhalanskyjb dkhalanskyjb force-pushed the dk-remove-linked-list branch from 03a42b9 to 7fe8543 Compare April 23, 2024 09:38
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 dkhalanskyjb force-pushed the dk-remove-linked-list branch from 7fe8543 to 70c9926 Compare May 21, 2024 13:15
@dkhalanskyjb
Copy link
Collaborator Author

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

@dkhalanskyjb
Copy link
Collaborator 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
Collaborator 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.

@dkhalanskyjb dkhalanskyjb force-pushed the dk-remove-linked-list branch from 6fa8981 to a025add Compare May 23, 2024 14:23
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.

1 participant