Optimize JobWrapper queueing ActiveJobs #35
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
From one angle it's an optimization, from another angle it's a bugfix (: We don't need to call ActiveJob::Base.deserialize during queueing the job, because we already have an instance of the class we can send methods to for finding out the #max_attempts or #queue_name. Teach JobWrapper how to take a job instance in and serialize it into job_data in that case, whilst still supporting being loaded from the database as a hash and calling deserialize lazily (existing flow). The potential bugfix is someone implementing a second-order effect in #deserialize like using it as a hook to setup ActiveSupport::CurrentAttributes for the job from persisted data. If we invoke #deserialize during enqueuing this can change CurrentAttributes values in the web request that's queueing the job.
smudge
reviewed
Jan 11, 2024
smudge
reviewed
Jan 11, 2024
caius
commented
Jan 12, 2024
Comment on lines
+21
to
+33
| ## [0.5.2] - 2023-10-19 | ||
| ### Fixed | ||
|
|
||
| - Fixed regression for Rails 7.1.1 not executing jobs, due to lazy loading change in Rails. | ||
| The `JobWrapper` needs to be loaded before `ActiveJob::Base` is loaded, but this wasn't | ||
| happening in the worker processes. Fix by extracting `JobWrapper` into it's own file and | ||
| loading when `Delayed` is loaded earlier in the process. | ||
|
|
||
| ## [0.5.1] - 2023-10-11 | ||
| ### Changed | ||
|
|
||
| - Explicitly test Rails 7.1 and Ruby 3.1, Ruby 3.2 to verify compatability. Previous | ||
| gems were compatible, but this release is now verified to be compatible. |
There was a problem hiding this comment.
@smudge I noticed the changelog wasn't up to date, so backfilled these entries based on the diffs/PR descriptions.
|
@smudge gentle ping? :) |
smudge
approved these changes
Jan 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a change to avoid calling
ActiveJob::Base#deserializeduring queueing the job whilst maintaining existing functionality.The change is in
JobWrapper, which previously only took in a hash ofjob_dataand knew how to deserialize it into a job object so we can interrogate it forqueue_nameormax_attempts, etc. To avoid the roundtrip through serialization, we changeJobWrapper#initializeto take in either the job object directly, or a hash as it did previously. When it's a hash (needed when running the job in the worker), it still deserializes it into a job object. When it's a job object just assign it to@jobdirectly and continue.We discovered this through having a second-order effect in
#deserializeusing it to trigger some logic as the job comes off the queue, but we were then seeing the logic run multiple times for one job execution. Tracked it back to theJobClass.perform_latercalling#deserializeand being majorly confused why it was deserialising in the web process.This will make enqueueing jobs faster too, because we don't need a roundtrip through
#serialize,#deserializeas well as allocating less objects. In practice this is likely a small effect, but we get it for free avoiding the behaviour above.Looking at all the built-in adapters in ActiveJob, as well as good_job and solid_queue, they all avoid calling
job.serializeduring the enqueuing, even those adapters that support invoking methods on the Job class instance like delayed does.From one angle it's an optimization, from another angle it's a bugfix (: