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

Remove SerializesModels for performance gains #196

Closed
wants to merge 1 commit into from
Closed

Remove SerializesModels for performance gains #196

wants to merge 1 commit into from

Conversation

mabdullahsari
Copy link

Hi 👋

This PR removes the SerializesModels trait from the base ProcessWebhookJob class.


Please allow me to explain the reasoning behind this change proposal. The documentation states (verbatim):

If your queued job accepts an Eloquent model in its constructor, only the identifier for the model will be serialized onto the queue. When the job is actually handled, the queue system will automatically re-retrieve the full model instance and its loaded relationships from the database. This approach to model serialization allows for much smaller job payloads to be sent to your queue driver.

While this behavior is fine for the vast majority of use cases out there, I think that free performance gains are left on the table by utilizing this trait within this particular context of webhooks. As you may already know, incoming webhooks are essentially events dispatched over Http, and by definition events are always immutable because they represent things that happened in the past. This immutable nature of events guarantees us that no changes to the serialized WebhookCall are going to occur by the time a queue worker picks up the job and starts processing it. Consequently, each processed job is no longer going to make an additional database query to retrieve the serialized model thus resulting in performance improvements.

I don't think that Job payload is going to be an issue, as most webhook payloads have a small footprint anyway.


This is definitely a breaking change, and I'm targeting main right now because there was no other option. I'm definitely glad to close and re-open the PR if a next or v4 branch is made.

Thanks 🙌

@freekmurze
Copy link
Member

Thanks for explaining. I'm going to pass on this as in the vast majority of cases having the SerializesModels trait won't pose a real-world problem.

I'll take a look again when working on a new major version.

@freekmurze freekmurze closed this Sep 25, 2023
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