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

[performance] http response encoding / writing improvements #2374

Conversation

NyaaaWhatsUpDoc
Copy link
Member

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc commented Nov 21, 2023

Description

This adds new wrapper functions within internal/api/util that handle response writing with content type / length setting. Where possible these will stream responses, and an all cases except for pre-prepared byte slices has the addition of pooled memory buffers for response encoding (including by JSON and XML). And multiple cases this also enables the use of pre-prepared JSON responses to reduce unneeded encoding for simple but known cases.

Checklist

Please put an x inside each checkbox to indicate that you've read and followed it: [ ] -> [x]

If this is a documentation change, only the first checkbox must be filled (you can delete the others if you want).

  • 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 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.

data io.Reader,
length int64,
) {
if length < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be <= instead of <? Do we always set length to -1 when we don't know it?

Copy link
Member Author

Choose a reason for hiding this comment

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

i mean a length of zero is a valid content-length, which might happen if say we just want to pass in an arbitrary reader + length (even if the reader contains zero data). -1 is a specifically invalid content-length so a useful flag to indicate non-standard behaviour

buf := getBuf()

// Wrap buffer in JSON encoder.
enc := json.NewEncoder(buf)
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to do SetEscapeHTML(false) on JSON encoders, since we don't really need escaping of HTML characters within the JSON in this context (API).

Copy link
Member Author

Choose a reason for hiding this comment

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

ooh i'm sure that should result in a small little performance boost too with less escape checking + handling to do 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

ah it seems some of our test JSON was expecting escaping, let me go through and fix those :p

Copy link
Member Author

Choose a reason for hiding this comment

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

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 good, very clean and tidy :)

@daenney
Copy link
Member

daenney commented Nov 27, 2023

I'm not sure I understand the change. Gin's JSON, and associated render methods, will do stuff like set the right content-type and I don't think I've ever observed the content-length missing either?

Never mind, reading comprehension suffers on Monday morning.

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc merged commit 74700cc into superseriousbusiness:main Nov 27, 2023
2 checks passed
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

3 participants