Skip to content

Background Job Manager (BJM) - replacement for BIO#356

Open
JimB123 wants to merge 2 commits into
valkey-io:unstablefrom
JimB123:bio-to-bjm
Open

Background Job Manager (BJM) - replacement for BIO#356
JimB123 wants to merge 2 commits into
valkey-io:unstablefrom
JimB123:bio-to-bjm

Conversation

@JimB123

@JimB123 JimB123 commented Apr 23, 2024

Copy link
Copy Markdown
Member

The BIO module is a source of minor annoyance. Background Job Manager (BJM) is designed as a modular replacement for BIO. Assuming there is positive response for this PR, the PR will be extended to convert existing BIO usage to BJM and remove BIO.

Motivations:

  • BIO (Background I/O) is a misnomer. I'd guess that this was originally a mechanism to perform an async flush on the AOF file, but it has since morphed into a mechanism for performing other kinds of background activities - like lazy flush - which have nothing to do with I/O
  • BIO isn't well layered. BIO should be a low-level utility for handling async background tasks. However the design of this utility requires that the "low-level" code have specific knowledge of the "high-level" application.
  • BIO isn't very modular. For each new type of background task, the BIO code must be altered. A new thread is created for each type of task. New definitions and logic are added for each new task.
  • Improve performance. BIO creates multiple threads, but doesn't use them effectively. There is 1 unique thread for each type of task. During a burst of lazy evictions, only the (single) eviction thread will be active while other BIO threads sit idle. During a lazy flush operation, 2 dictionaries (main and expire) are processed sequentially rather than in parallel.
  • Reduced memory usage. BIO uses a list to maintain the queue of jobs. The new Fifo uses much less memory (and is more performant) if there's a large burst of jobs.

PR Contents:

  • Fifo - presents a simple FIFO queue, it is over 50% more space efficient and over 50% faster than using Redis list. It's delivered as an independent, modular, & reusable utility data structure.
  • BJM provides a simple fire-and-forget interface to have something done by a background thread. It maintains a fixed/configurable quantity of threads (rather than one per "job type"). This prevents having too many active background threads and also allows faster processing for a large number of jobs of the same job type.

Originally proposed to Redis and stalled here: redis/redis#12029

Note especially that in the original proposal, there was some concern about WAITAOF which currently relies on BIO to serialize 2 independent background jobs. There were 3 possible solutions mentioned:

  • Retain BIO for this case
  • Modify the WAITAOF code so that it enqueues a single sequenced job rather than 2 jobs
  • Enhance the proposed BJM mechanism with "futures" to provide a mechanism for synchronization

Signed-off-by: Jim Brunner <brunnerj@amazon.com>
@codecov

codecov Bot commented Apr 23, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 207 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.11%. Comparing base (393c8fd) to head (2e9a5ee).
⚠️ Report is 1593 commits behind head on unstable.

Files with missing lines Patch % Lines
src/bjm.c 0.00% 119 Missing ⚠️
src/fifo.c 0.00% 88 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #356      +/-   ##
============================================
- Coverage     68.39%   68.11%   -0.28%     
============================================
  Files           108      110       +2     
  Lines         61562    61769     +207     
============================================
- Hits          42107    42076      -31     
- Misses        19455    19693     +238     
Files with missing lines Coverage Δ
src/fifo.c 0.00% <0.00%> (ø)
src/bjm.c 0.00% <0.00%> (ø)

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Jim Brunner <brunnerj@amazon.com>

@madolson madolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I've been ignoring this for ever but I think it's worth pulling in. Some inconsistencies in style and some odd name choices, but I'm happy to sit down and chat to get this merged.

Comment thread src/bjm.c

info = sdscatprintf(info,
"# BackgroundJobManager\r\n"
"bjm_num_threads:%d\r\n"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"bjm_num_threads:%d\r\n"
"background_job_num_threads:%d\r\n"

I'm going to bring up that BJM means something else, and would prefer we not abbreviate it. I personally never liked the previous RIO or BIO abbreviations, because it was never clear what they meant.

Comment thread src/bjm.h
Comment on lines +11 to +12
/* Initialize BJM with the requested number of background threads.
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Move the documentation to the .c files to better follow coding conventions.

Comment thread src/fifo.h
Comment on lines +37 to +38
// Returns a new Fifo, containing all of the items from "q". "q" remains valid, but becomes empty.
// This is an O(1) operation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By coding convention multi line comments need to do /* */

Comment thread src/fifo.h

// Returns a new Fifo, containing all of the items from "q". "q" remains valid, but becomes empty.
// This is an O(1) operation.
Fifo * fifoPopAll(Fifo *q);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Fifo * fifoPopAll(Fifo *q);
Fifo *fifoPopAll(Fifo *q);

Comment thread src/fifo.c


// Look at the first item in the queue (without removing it).
// NOTE: asserts if the queue is empty.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still think this is a weird and counter intuitive assumption. Both that the object can't be NULL and that is asserts on length.

Comment thread src/bjm.c

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would prefer to not use an acronym and just spell out bjm.

@dvkashapov

Copy link
Copy Markdown
Member

@JimB123 What do you think about getting this up to speed? Are you still interested in updating it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Implementation

Development

Successfully merging this pull request may close these issues.

3 participants