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

try to fix ubuntu18 installation failure #4643

Draft
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

meiravgri
Copy link
Collaborator

Describe the changes in the pull request

A clear and concise description of what the PR is solving, including:

  1. The current state briefly
  2. What is the change
  3. Adding the outcome (changed state)

Which issues this PR fixes

  1. #...
  2. MOD...

Main objects this PR modified

  1. ...
  2. ...

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

meiravgri and others added 30 commits February 12, 2024 13:03
wait and drain are modified accordingly.
rename priority_queue->num_threads_working to num_threads_not_idle

uncomment LOG

remove script
Co-authored-by: GuyAv46 <47632673+GuyAv46@users.noreply.github.com>
Thcount used to lock the code block in thread_do where we realize the state is THPOOL_TERMINATE_WHEN_EMPTY and the jobq, and want to inform the other threads. However, this lock was not enough to ensure that thread won't go to sleep after the signal has already been sent.

for example, once the queue is empty,
for thread1 both state = THPOOL_TERMINATE_WHEN_EMPTY and jobq is empty,
switch to thread2: try to pull a job from the queue, is_empty & should run both true, back to thread 1: change should_run and broadcast all other threads
switch to thread2: sleep on the cond

we need to make sure that both code block are protected by the same mutex:
1. thread_do changes the thpool state and signal the other threads
2. jobq_pull check if the jobq is empty and should_run and sleep on the cond.

we lock these code blocks with jobqueues_rwmutex.
thcount_lock is now redundant and the variable it used to guard can be atomic instead.
… approximate way to signal drain that all threads are idle. However, drain is more likly to wakeup due to timeout, since idealy, all threads are working as long as there are jobs in jobq.

As for wait function, the threads were signaling that all are ideal before checking the queue length, so again, it doesn't indicate the real state of the queue (num_working might be 0, but there are still jobs in the queue)

the condition variable was replaced with a sleeping and waking up after 100 ms.

also, num working is increased if we pulled a job, and decreased when the job is done.
change back to num_threads_working

added description to has_jobs and should_run

remove error checking from redisearch_thpool_create

add num_threads_alive to stats

remove unused priority_queue_len

added a test to test terminate when empty
added a util to create any class of thpool tests
unify if (job_p)  in thread_do

remove unused var from sleep_and_set in test_cpp_thpool
@meiravgri meiravgri marked this pull request as draft May 16, 2024 13:24
Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.97%. Comparing base (78409fd) to head (7bf18cd).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4643      +/-   ##
==========================================
- Coverage   85.98%   85.97%   -0.01%     
==========================================
  Files         190      190              
  Lines       34517    34517              
==========================================
- Hits        29678    29677       -1     
- Misses       4839     4840       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

1 participant