Skip to content

Conversation

@ianchen0119
Copy link
Contributor

@ianchen0119 ianchen0119 commented Nov 7, 2025

Summary of all changes:

1. Data structure refactor:

  • Replaced the original ring buffer task pool (managed by taskPoolHead/taskPoolTail plus manual shifting) with an in‑place binary min‑heap stored in the preallocated slice taskPool[0:taskPoolCount].
  • Implemented heap operations: heapSiftUp, heapSiftDown, and a comparator wired to existing lessQueuedTask (ordering by Deadline, then Timestamp, then Pid).
  • Removed obsolete fields taskPoolHead and taskPoolTail from GthulhuPlugin.

2. Insertion & retrieval logic:

  • insertTaskToPool now appends the new task at the logical end (index = taskPoolCount) and performs sift‑up.
  • getTaskFromPool now pops the root (minimum) and replaces it with the last element followed by sift‑down.
  • drainQueuedTask adjusted to respect capacity using taskPoolCount < taskPoolSize-1 instead of ring buffer arithmetic.

3. Concurrency safety:

  • Added poolMu (sync.Mutex) to protect heap mutations (insert, pop, capacity checks).
  • Added strategyMu (sync.RWMutex) to protect strategyMap reads (applySchedulingStrategy, getTaskExecutionTime) and writes (UpdateStrategyMap).
  • Wrapped all relevant sections with appropriate locking; minimized lock scope where possible.

4. Scheduling strategy handling:

  • applySchedulingStrategy and getTaskExecutionTime now use read locks; UpdateStrategyMap rebuilds and swaps the map under a write lock.
  • minVruntime remains unsynchronized beyond the existing enqueue path (could be guarded if updated from multiple goroutines in the future).

5. Testing additions:

  • Added TestMinHeapOrderByDeadline to verify that tasks are popped in ascending (Deadline, Timestamp, Pid) order from the heap.
  • Existing tests still pass after structural changes (heap introduction + locking).

6. Attempted change (reverted):

  • A temporary modification made updatedEnqueueTask return Vtime as Deadline; final state reverted to returning 0. Currently Deadline is only meaningfully set when manually injected (e.g. in the new heap ordering test). Future improvement could reintroduce using computed Vtime (or another scheduling key) for real ordering.

7. Performance implications:

  • Insertion complexity reduced from O(n) (linear search + shifting) to O(log n).
  • Removal remains O(log n) instead of previous O(1) head pop, but overall throughput improves for high insertion rates.
  • Memory layout unchanged (still preallocated array of size taskPoolSize); no extra allocations per insert.

8. Concurrency implications:

  • Eliminated potential data races between background strategy-fetch goroutine and scheduling path.
  • Heap integrity protected against concurrent inserts/selects.
  • Locks are coarse but short‑lived; contention acceptable unless extremely high task turnover—could later optimize with a lock-free or sharded structure if needed.

9. Code cleanliness:

  • Removed unused ring buffer indices, simplifying mental model.
  • Kept existing lessQueuedTask comparator to avoid altering scheduling semantics.

Copilot AI review requested due to automatic review settings November 7, 2025 18:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the task pool from a circular buffer to a binary min-heap implementation and adds thread-safety with mutexes.

  • Replaced circular buffer (head/tail pointers) with a proper binary min-heap using sift-up and sift-down operations
  • Added mutex protection (poolMu and strategyMu) for concurrent access to task pool and strategy map
  • Added a new test to verify heap ordering by deadline, timestamp, and PID

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
plugin/gthulhu/gthulhu.go Replaces circular buffer with min-heap implementation, adds mutexes for thread safety, and implements heap operations (sift-up/sift-down)
plugin/gthulhu/gthulhu_test.go Adds test case to verify min-heap ordering by deadline with timestamp and PID as tie-breakers
Comments suppressed due to low confidence (1)

plugin/gthulhu/gthulhu.go:181

  • The function updatedEnqueueTask always returns 0, but the return value is assigned to task.Deadline in line 150. This means all tasks will have a deadline of 0, which breaks the heap ordering logic. The function should likely return t.Vtime to use the computed virtual runtime as the deadline.
	return 0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ianchen0119 ianchen0119 merged commit cf098e5 into main Nov 7, 2025
1 check passed
@ianchen0119 ianchen0119 deleted the feat/gthulhu-perf-enhancement branch November 7, 2025 18:51
ianchen0119 added a commit to Gthulhu/Gthulhu that referenced this pull request Nov 7, 2025
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