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.

Sign up for GitHub

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

Jump to bottom

[performance] massively improved ActivityPub delivery worker efficiency #2812

Merged
merged 35 commits into from
Apr 11, 2024
Merged

[performance] massively improved ActivityPub delivery worker efficiency #2812

merged 35 commits into from
Apr 11, 2024

Conversation

NyaaaWhatsUpDoc
Copy link
Member

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc commented Apr 5, 2024 •
edited
Loading

Description

This replaces our current transport delivery routine to instead push deliveries to a concurrency-safe queue, which is pulled from by a fixed-size pool of delivery workers. These delivery workers are optimized (unlike currently) to prefer new attempted deliveries over backlogged erroring (i.e. retry-backoff) deliveries, which hugely optimizes both the amount of time our goroutines spend and the actual number of them, so they're doing useful delivery work and not performing retry-backoffs in serial.

Also sneaks in a lil change to use errorsv2.AsV2() instead of errors.As() in a number of places because it's much nicer working with generic types, and likely faster instead of the existing reflect logic.

Checklist

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have not leveraged AI to create the proposed changes.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

@NyaaaWhatsUpDoc
add delivery worker type that pulls from queue to httpclient package
aa01437
@NyaaaWhatsUpDoc
finish up some code commenting, bodge a vendored activity library cha…
8aede54 
…nge, integrate the deliverypool changes into transportcontroller
@NyaaaWhatsUpDoc
hook up queue deletion logic
8fdd5b1
@NyaaaWhatsUpDoc
support deleting queued http requests by target ID
d7e14d5
@NyaaaWhatsUpDoc
don't index APRequest by hostname in the queue
033b2e7
@NyaaaWhatsUpDoc
use gorun
73ef636
@NyaaaWhatsUpDoc
use the original context's values when wrapping msg type as delivery{}
3439ce7
@NyaaaWhatsUpDoc
actually log in the AP delivery worker ...
a8ea1fe
@NyaaaWhatsUpDoc
add uncommitted changes
3c9451b
@NyaaaWhatsUpDoc
use errors.AsV2()
175c870
@NyaaaWhatsUpDoc
use errorsv2.AsV2()
d899239
@NyaaaWhatsUpDoc
finish adding some code comments, add bad host handling to delivery w…
f70ef72 
…orkers
@NyaaaWhatsUpDoc
slightly tweak deliveryworkerpool API, use advanced sender multiplier
7080930
@NyaaaWhatsUpDoc
remove PopCtx() method, let others instead rely on Wait()
ba60397
@NyaaaWhatsUpDoc
shuffle things around to move delivery stuff into transport/ subpkg
a3bd01d
@NyaaaWhatsUpDoc
remove dead code
5d6ffd7
@NyaaaWhatsUpDoc
formatting
9ce0ffc
@NyaaaWhatsUpDoc
validate request before queueing for delivery
0f4c425
@NyaaaWhatsUpDoc
finish adding code comments, fix up backoff code
9264a82
@NyaaaWhatsUpDoc
finish adding more code comments
fd7c958
@NyaaaWhatsUpDoc
clamp minimum no. senders to 1
e06617d
@NyaaaWhatsUpDoc
add start/stop logging to delivery worker, some slight changes
e17474f
@NyaaaWhatsUpDoc
remove double logging
500e755
@NyaaaWhatsUpDoc
use worker ptrs
f17ef91
@NyaaaWhatsUpDoc
expose the embedded log fields in httpclient.Request{}
4811ded
@NyaaaWhatsUpDoc
ensure request context values are preserved when updating ctx
59ec876
@NyaaaWhatsUpDoc
add delivery worker tests
1b9308e
@NyaaaWhatsUpDoc
fix linter issues
51f6fd5
@NyaaaWhatsUpDoc
ensure delivery worker gets inited in testrig
39e6d05
@NyaaaWhatsUpDoc
fix tests to delivering messages to check worker delivery queue
3880020
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc mentioned this pull request Apr 8, 2024
Merged
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc changed the title [feature] indexable http client queue and fixed http client worker pool [feature] massively improved ActivityPub delivery worker efficiency Apr 8, 2024
NyaaaWhatsUpDoc
NyaaaWhatsUpDoc commented Apr 8, 2024
View reviewed changes
go.mod Show resolved Hide resolved
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc marked this pull request as ready for review April 8, 2024 13:20
tsmethurst
tsmethurst approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

Looks great to me :) Good work! Once 0.15.0 is out let's squerge this.

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc changed the title [feature] massively improved ActivityPub delivery worker efficiency [performance] massively improved ActivityPub delivery worker efficiency Apr 8, 2024
@tsmethurst tsmethurst merged commit a483bd9 into superseriousbusiness:main Apr 11, 2024
2 checks passed
nyarla pushed a commit to nyarla/gotosocial-modded that referenced this pull request Jun 19, 2024
@NyaaaWhatsUpDoc @nyarla
[performance] massively improved ActivityPub delivery worker efficien…
763c8b5 
…cy (superseriousbusiness#2812)

* add delivery worker type that pulls from queue to httpclient package

* finish up some code commenting, bodge a vendored activity library change, integrate the deliverypool changes into transportcontroller

* hook up queue deletion logic

* support deleting queued http requests by target ID

* don't index APRequest by hostname in the queue

* use gorun

* use the original context's values when wrapping msg type as delivery{}

* actually log in the AP delivery worker ...

* add uncommitted changes

* use errors.AsV2()

* use errorsv2.AsV2()

* finish adding some code comments, add bad host handling to delivery workers

* slightly tweak deliveryworkerpool API, use advanced sender multiplier

* remove PopCtx() method, let others instead rely on Wait()

* shuffle things around to move delivery stuff into transport/ subpkg

* remove dead code

* formatting

* validate request before queueing for delivery

* finish adding code comments, fix up backoff code

* finish adding more code comments

* clamp minimum no. senders to 1

* add start/stop logging to delivery worker, some slight changes

* remove double logging

* use worker ptrs

* expose the embedded log fields in httpclient.Request{}

* ensure request context values are preserved when updating ctx

* add delivery worker tests

* fix linter issues

* ensure delivery worker gets inited in testrig

* fix tests to delivering messages to check worker delivery queue

* update error type to use ptr instead of value receiver

* fix test calling Workers{}.Start() instead of testrig.StartWorkers()

* update docs for advanced-sender-multiplier

* update to the latest activity library version

* add comment about not using httptest.Server{}
nyarla pushed a commit to nyarla/gotosocial-modded that referenced this pull request Jun 19, 2024
@NyaaaWhatsUpDoc @nyarla
[performance] massively improved ActivityPub delivery worker efficien…
9867e91 
…cy (superseriousbusiness#2812)

* add delivery worker type that pulls from queue to httpclient package

* finish up some code commenting, bodge a vendored activity library change, integrate the deliverypool changes into transportcontroller

* hook up queue deletion logic

* support deleting queued http requests by target ID

* don't index APRequest by hostname in the queue

* use gorun

* use the original context's values when wrapping msg type as delivery{}

* actually log in the AP delivery worker ...

* add uncommitted changes

* use errors.AsV2()

* use errorsv2.AsV2()

* finish adding some code comments, add bad host handling to delivery workers

* slightly tweak deliveryworkerpool API, use advanced sender multiplier

* remove PopCtx() method, let others instead rely on Wait()

* shuffle things around to move delivery stuff into transport/ subpkg

* remove dead code

* formatting

* validate request before queueing for delivery

* finish adding code comments, fix up backoff code

* finish adding more code comments

* clamp minimum no. senders to 1

* add start/stop logging to delivery worker, some slight changes

* remove double logging

* use worker ptrs

* expose the embedded log fields in httpclient.Request{}

* ensure request context values are preserved when updating ctx

* add delivery worker tests

* fix linter issues

* ensure delivery worker gets inited in testrig

* fix tests to delivering messages to check worker delivery queue

* update error type to use ptr instead of value receiver

* fix test calling Workers{}.Start() instead of testrig.StartWorkers()

* update docs for advanced-sender-multiplier

* update to the latest activity library version

* add comment about not using httptest.Server{}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsmethurst tsmethurst tsmethurst approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NyaaaWhatsUpDoc @tsmethurst