-
Notifications
You must be signed in to change notification settings - Fork 743
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
Feature/bca/perf better send time #2235
Conversation
9664417
to
301ff39
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a first static review. That's amazing work!
Will try to use the code during the week-end
@@ -93,17 +91,6 @@ internal class DefaultSendService @AssistedInject constructor( | |||
} | |||
|
|||
// For test only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also remove this comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
private val queuedTaskFactory: QueuedTaskFactory, | ||
private val taskExecutor: TaskExecutor, | ||
private val memento: QueueMemento | ||
) : Thread("SENDER_THREAD_SID_${sessionParams.credentials.sessionId()}") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested the signout, if there are unsent Events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the session is closed, the send processor will be interrupted thus pending messages won't be sent.
* reschedule them (and only them) on next restart | ||
*/ | ||
internal class QueueMemento @Inject constructor(context: Context, | ||
sessionParams: SessionParams, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can inject @sessionid instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* Info that need to be persisted by the sender thread | ||
* With polymorphic moshi parsing | ||
*/ | ||
interface TaskInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
) : TaskInfo | ||
|
||
@JsonClass(generateAdapter = true) | ||
data class FallbackTaskInfo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all those 3 data classes should have internal
modifier I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
27b1812
to
dc8a6cc
Compare
Fixes #516 |
Improve send time performances why removing usage of workers to send events.
On some devices heavy delay were experienced when sending events ( 5s, 6s, 20s...). It's touching some devices and not others and is still a bit random.
Workers by them selves are adding a bit of delay before launching, and in addition we have to rely on database to get up to date localEchos, and such transactions are sometime very slow
Example of improvement of send time (top is with worker, bottom is with new implementation)
The current implem just uses a sender thread, that consumes send tasks in order.
Media messages are still using workers
In addition some improvement have been made on timeline with broader support of UI local echos
** Known Limitation