Improve performance for Probabilistic priority queueing #344
Conversation
…rove Origin/http offer set perf improve
fetch from upstream - eom
…lenhattan86/scheduler into probabilistic_priority_queueing
…ticPriorityAssigner.java Co-authored-by: Renán I. Del Valle <github@ridv.xyz>
…ticPriorityAssigner.java Co-authored-by: Renán I. Del Valle <github@ridv.xyz>
…ticPriorityAssigner.java Co-authored-by: Renán I. Del Valle <github@ridv.xyz>
…ticPriorityAssigner.java Co-authored-by: Renán I. Del Valle <github@ridv.xyz>
src/main/java/io/github/aurora/scheduler/scheduling/ProbabilisticPriorityAssigner.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/aurora/scheduler/scheduling/ProbabilisticPriorityAssignerModule.java
Show resolved
Hide resolved
| @@ -54,7 +55,8 @@ | |||
| private static final Logger LOG = LoggerFactory. | |||
| getLogger(ProbabilisticPriorityAssigner.class); | |||
|
|
|||
| private final Storage storage; | |||
| private static Iterable<IScheduledTask> pendindTasks = new LinkedList<>(); | |||
There was a problem hiding this comment.
You should use an ArrayList here for better performance, this is on a fast path and the size of the list can get significant putting pressure on memory pages. while iterating
There was a problem hiding this comment.
Also what makes you use a static variable here?
| @@ -131,8 +132,9 @@ boolean isScheduled(Set<Integer> prioritySet, int priority) { | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| Iterable<IScheduledTask> getPendingTasks() { | |||
| return Storage.Util.fetchTasks(storage, Query.unscoped().byStatus(ScheduleStatus.PENDING)); | |||
| public static synchronized void fetchPendingTasks(Storage storage) { | |||
There was a problem hiding this comment.
@VisibleForTesting but I don't see any tests calling this?
There was a problem hiding this comment.
I don't see the need for this method at all in this class. Your PendingTasksFetcher should call Storage.Util.fetchTasks(storage, Query.unscoped().byStatus(ScheduleStatus.PENDING)); directly from its run() method, and the responsibility of fetchPendingTasks should be with that class, not this. The objects here should only use methods from that class. This class is responsible for assigning priorities, not for fetching tasks as a public method.
|
|
||
| import org.apache.aurora.scheduler.storage.Storage; | ||
|
|
||
| public class TaskFetcher implements Runnable { |
There was a problem hiding this comment.
This seems to fetch only pending tasks, why is it named TaskFetcher?
There was a problem hiding this comment.
You seem to be having multiple classes doing similar things. You should use a single executor to fetch these tasks with whatever filters are needed and use them from different classes. Having multiple executors for the same thing is not a good idea.
src/main/java/io/github/aurora/scheduler/scheduling/ProbabilisticPriorityAssignerModule.java
Show resolved
Hide resolved
|
|
||
| @Override | ||
| protected void startUp() { | ||
| executor.scheduleAtFixedRate(taskFetcher, 0, taskFetchIntervalMs, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
What is the taskFetchIntervalMs here?
Description:
For each scheduling latency, we need to have a set of priorites fromt the pending tasks.
if we do it for every scheduling cycle, it is not scalable because the latency grows with respect o the number of tasks in task_store.
Instead of doing this for every scheduling cycle, we can pull the pending tasks every
probabilistic_priority_assigner_task_fetch_interval. We don't need the accurate number becauseTesting Done:
unit test.
integration test.
performance test.