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

Use a state machine to implement AsyncTimeout #1397

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

swankjesse
Copy link
Member

Previously we had 3 boolean state variables to track 4 possible states. That was more complex than necessary.

head = AsyncTimeout()
Watchdog().start()
}
private fun insertIntoQueue(node: AsyncTimeout, timeoutNanos: Long, hasDeadline: Boolean) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Most of the changes in this function are just whitespace. I moved withLock {} to the caller.

Screenshot 2023-12-16 at 10 03 42 AM

}
prev = prev.next
}

// The node wasn't found in the linked list: it must have timed out!
return true
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we keep a state machine we don’t need to traverse the linked list to learn this

Previously we had 3 boolean state variables to track 4 possible
states. That was more complex than necessary.
@@ -57,11 +56,6 @@ public class okio/AsyncTimeout : okio/Timeout {
public final fun withTimeout (Lkotlin/jvm/functions/Function0;)Ljava/lang/Object;
}

public final class okio/AsyncTimeout$Companion {
Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, I think these snuck in by accident. I don’t believe they belong in the public API, and I don’t think anyone ever used them.

@swankjesse swankjesse merged commit ac9cd96 into master Dec 16, 2023
11 checks passed
@swankjesse swankjesse deleted the jwilson.1216.state_machine branch December 16, 2023 19:53
@yyaroshevich
Copy link

Hello,

I'm not sure if this is the right place to ask, but I hope @swankjesse, as the author of several timeout-related PRs, might be able to help. Please know that I ask this question with the utmost respect and curiosity, and it is not meant to be critical or blaming in any way.

Is there a specific reason why AsyncTimeout is still implemented as a manual linked list with a backing thread instead of using ScheduledThreadPoolExecutor in 2024? I understand that this class was initially written before JDK 8, but now it seems that much of the complex code could be simplified by using ScheduledThreadPoolExecutor.

I'm asking because I've observed that list manipulations, such as linear time insertions into the linked list, have become a bottleneck in a back-end service that heavily relies on OkHttpClient and probably it is bottleneck in other apps which heavily uses OkHttpClient.

Thank you for any insights you can provide.

Best regards, Yury

@swankjesse
Copy link
Member Author

@yyaroshevich that’s a good point!

Wanna open a tracking bug for us to consider switching? At the time of creation we optimized for fewest allocations, which were particularly slow on Android.

One other option that we should investigate – could we accomplish better performance by searching backwards instead of forwards in the list? If all of the timeouts are symmetric (they often are), then inserts will usually be at the end. We could reduce contention without increasing allocation?

@yyaroshevich
Copy link

Wanna open a tracking bug for us to consider switching?

Sure, I've opened ticket where we can proceed discussion #1488. Thanks a lot!

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

4 participants