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

Improve statics re-initialization #59

Merged
merged 3 commits into from
Jan 10, 2025
Merged

Improve statics re-initialization #59

merged 3 commits into from
Jan 10, 2025

Conversation

sellmair
Copy link
Member

No description provided.

- Statics will only be re-initialized if the clinit method actually changed
- Statics will re-initialize in topological dependency order

fixes #39
return try {
block().toLeft()
} catch (t: Throwable) {
if (t is CancellationException) throw t

Choose a reason for hiding this comment

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

Sorry, you didn't ask, but I got distracted by this lil nitpick, while indeed mostly what you need. There are cases where this might start throwing rogue cancellation exceptions. This becomes increasingly less common in properly structured concurrency (code that does not pass around CoroutineScopes and/or Jobs).
But if you want to be extra safe, you should use ensureActive() here instead.
I usually refer to this read:
https://betterprogramming.pub/the-silent-killer-thats-crashing-your-coroutines-9171d1e8f79b

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good catch. I skimmed over the article and will read it more carefully today evening.
However, since this Try is not a suspend function, I hope that this should just act as 'thin wall' and the rogue cancellation will be handled similar to the Try being absent.

Comment on lines 108 to 110
var index = 0

val nodes = mapTo(ArrayList(size * 2)) { value -> Node(value, index++) }

Choose a reason for hiding this comment

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

Again, not the feedback you asked for 😅, but mapIndexedTo exists :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thank you ☺️

var index = 0

val nodes = mapTo(ArrayList(size * 2)) { value -> Node(value, index++) }
val nodesMap = nodes.associateByTo(LinkedHashMap(nodes.size)) { it.value }

Choose a reason for hiding this comment

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

Does this need to be linked?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but interestingly, the benchmark prefers LinkedHashMap here 🤷 That was a surprise to me.

@sellmair sellmair enabled auto-merge (rebase) January 10, 2025 13:00
@sellmair sellmair disabled auto-merge January 10, 2025 13:48
@sellmair sellmair merged commit 1868ca2 into master Jan 10, 2025
4 checks passed
@sellmair sellmair deleted the sellmair branch January 10, 2025 13:48
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