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

reduce relaxedBodyCanonicalizer allocations #46

Closed
pierrre opened this issue Mar 29, 2021 · 3 comments · Fixed by #47
Closed

reduce relaxedBodyCanonicalizer allocations #46

pierrre opened this issue Mar 29, 2021 · 3 comments · Fixed by #47

Comments

@pierrre
Copy link
Contributor

pierrre commented Mar 29, 2021

In my application, nearly 50% of my memory allocations (in count) are caused by relaxedBodyCanonicalizer.

type relaxedBodyCanonicalizer struct {
w io.Writer
crlfBuf []byte
wspBuf []byte
written bool
crlfFixer crlfFixer
}
func (c *relaxedBodyCanonicalizer) Write(b []byte) (int, error) {
written := len(b)
b = c.crlfFixer.Fix(b)
canonical := make([]byte, 0, len(b))
for _, ch := range b {
if ch == ' ' || ch == '\t' {
c.wspBuf = append(c.wspBuf, ch)
} else if ch == '\r' || ch == '\n' {
c.wspBuf = nil
c.crlfBuf = append(c.crlfBuf, ch)
} else {
if len(c.crlfBuf) > 0 {
canonical = append(canonical, c.crlfBuf...)
c.crlfBuf = nil
}
if len(c.wspBuf) > 0 {
canonical = append(canonical, ' ')
c.wspBuf = nil
}

Especially lines 146 and 149.

I think this code could be optimized.
Currently each call to append causes a new memory allocation and requires to copy the memory.
Because the slices are reset to nil.
It doesn't reuse the memory previously allocated.
The solution: do c.wspBuf = c.wspBuf[:0] instead of c.wspBuf = nil.
Same for crlfBuf.

WDYT ?

@emersion
Copy link
Owner

emersion commented Mar 29, 2021

wspBuf could be replaced with a wsp bool field, since we never use the actual value. Your suggestion looks like a good idea for crlfBuf.

Patches welcome!

@pierrre
Copy link
Contributor Author

pierrre commented Mar 29, 2021

wspBuf could be replaced with a wsp bool field

Yes I noticed that too 😅

I'll work on it tomorrow.

@pierrre
Copy link
Contributor Author

pierrre commented Mar 30, 2021

Here is the PR #47

emersion pushed a commit that referenced this issue Mar 30, 2021
- reslice crlfBuf to an empty slice, instead of reset it to nil
- change wspBuf to a boolean, the content was not used

Fixes #46
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 a pull request may close this issue.

2 participants