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

slog: faster and less memory-consumption #28621

Merged
merged 8 commits into from Dec 1, 2023
Merged

slog: faster and less memory-consumption #28621

merged 8 commits into from Dec 1, 2023

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Nov 29, 2023 •
edited

Some previous discussion here: jwasinger#8

These changes improves the performance of the non-coloured terminal formatting, quite a lot.

name               old time/op    new time/op    delta
TerminalHandler-8    10.2µs ±15%     5.4µs ± 9%  -47.02%  (p=0.008 n=5+5)

name               old alloc/op   new alloc/op   delta
TerminalHandler-8    2.17kB ± 0%    0.40kB ± 0%  -81.46%  (p=0.008 n=5+5)

name               old allocs/op  new allocs/op  delta
TerminalHandler-8      33.0 ± 0%       5.0 ± 0%  -84.85%  (p=0.008 n=5+5)

I tried to somewhat organize the commits, but the it might still be a bit chaotic. Some core insights:

  • The function terminalHandler.Handl uses a mutex, and writes all output immediately to 'upstream'. Thus, it can reuse a scratch-buffer every time.
  • This buffer can be propagated internally, making all the internal formatters either write directly to it,
  • OR, make use of the tmp := buf.AvailableBuffer() in some cases, where a byte buffer "extra capacity" can be temporarily used.
  • The slog package uses Attr by value. It makes sense to minimize operating on them, since iterating / collecting into a new slice, iterating again etc causes copy-on-heap. Better to operate on them only once.
  • If we want to do padding, it's better to copy from a constant space-buffer than to invoke bytes.Repeat every single time.

Todo:

  • bytes.AvailableBuffer was introduced in go1.21 use something else for now, instead.

holiman and others added 5 commits November 29, 2023 08:36
@holiman
log: better benchmark
0c8b6ca
@holiman
log: improve benchmark
4b6d32d
@holiman
log: lots of optimizations to terminal output
99f3e24 
- implement custom time writer,
- better padding with spaces
- better use of spaces
- log: more efficient handling of slog.Value
- log: utilize scratch-buffer in terminal handler
- log: use scratch-space for value formatting
@holiman
log: save some cpu by simplifying coloring, avoid many cases of escaping
d929f63
@jwasinger @holiman
remove commented code and no-op space
632a3f8
@holiman holiman mentioned this pull request Nov 29, 2023
Merged
@holiman holiman added this to the 1.13.6 milestone Nov 29, 2023
jwasinger
jwasinger approved these changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@jwasinger jwasinger left a comment

Choose a reason for hiding this comment

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

Other than a few commented lines of code throughout the PR, LGTM

@holiman holiman merged commit dd0d0a2 into ethereum:master Dec 1, 2023
2 checks passed
@holiman holiman deleted the faster_slog branch December 5, 2023 10:56
fearlessfe pushed a commit to fearlessfe/go-ethereum that referenced this pull request Dec 11, 2023
@holiman
slog: faster and less memory-consumption (ethereum#28621)
880843b 
These changes improves the performance of the non-coloured terminal formatting, _quite a lot_. 

```
name               old time/op    new time/op    delta
TerminalHandler-8    10.2µs ±15%     5.4µs ± 9%  -47.02%  (p=0.008 n=5+5)

name               old alloc/op   new alloc/op   delta
TerminalHandler-8    2.17kB ± 0%    0.40kB ± 0%  -81.46%  (p=0.008 n=5+5)

name               old allocs/op  new allocs/op  delta
TerminalHandler-8      33.0 ± 0%       5.0 ± 0%  -84.85%  (p=0.008 n=5+5)
```

I tried to _somewhat_ organize the commits, but the it might still be a bit chaotic. Some core insights: 

- The function `terminalHandler.Handl` uses a mutex, and writes all output immediately to 'upstream'. Thus, it can reuse a scratch-buffer every time. 
- This buffer can be propagated internally, making all the internal formatters either write directly to it,
- OR, make  use of the `tmp := buf.AvailableBuffer()` in some cases, where a byte buffer "extra capacity" can be temporarily used. 
- The `slog` package  uses `Attr` by value. It makes sense to minimize operating on them, since iterating / collecting into a new slice, iterating again etc causes copy-on-heap. Better to operate on them only once. 
- If we want to do padding, it's better to copy from a constant `space`-buffer than to invoke `bytes.Repeat` every single time.
@dnhn dnhn mentioned this pull request Dec 18, 2023
Merged
Doozers pushed a commit to kilnfi/pgeth that referenced this pull request Dec 22, 2023
@holiman @Doozers
slog: faster and less memory-consumption (ethereum#28621)
cd6ed25 
These changes improves the performance of the non-coloured terminal formatting, _quite a lot_. 

```
name               old time/op    new time/op    delta
TerminalHandler-8    10.2µs ±15%     5.4µs ± 9%  -47.02%  (p=0.008 n=5+5)

name               old alloc/op   new alloc/op   delta
TerminalHandler-8    2.17kB ± 0%    0.40kB ± 0%  -81.46%  (p=0.008 n=5+5)

name               old allocs/op  new allocs/op  delta
TerminalHandler-8      33.0 ± 0%       5.0 ± 0%  -84.85%  (p=0.008 n=5+5)
```

I tried to _somewhat_ organize the commits, but the it might still be a bit chaotic. Some core insights: 

- The function `terminalHandler.Handl` uses a mutex, and writes all output immediately to 'upstream'. Thus, it can reuse a scratch-buffer every time. 
- This buffer can be propagated internally, making all the internal formatters either write directly to it,
- OR, make  use of the `tmp := buf.AvailableBuffer()` in some cases, where a byte buffer "extra capacity" can be temporarily used. 
- The `slog` package  uses `Attr` by value. It makes sense to minimize operating on them, since iterating / collecting into a new slice, iterating again etc causes copy-on-heap. Better to operate on them only once. 
- If we want to do padding, it's better to copy from a constant `space`-buffer than to invoke `bytes.Repeat` every single time.
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
@holiman @Dergarcon
slog: faster and less memory-consumption (ethereum#28621)
023305e 
These changes improves the performance of the non-coloured terminal formatting, _quite a lot_. 

```
name               old time/op    new time/op    delta
TerminalHandler-8    10.2µs ±15%     5.4µs ± 9%  -47.02%  (p=0.008 n=5+5)

name               old alloc/op   new alloc/op   delta
TerminalHandler-8    2.17kB ± 0%    0.40kB ± 0%  -81.46%  (p=0.008 n=5+5)

name               old allocs/op  new allocs/op  delta
TerminalHandler-8      33.0 ± 0%       5.0 ± 0%  -84.85%  (p=0.008 n=5+5)
```

I tried to _somewhat_ organize the commits, but the it might still be a bit chaotic. Some core insights: 

- The function `terminalHandler.Handl` uses a mutex, and writes all output immediately to 'upstream'. Thus, it can reuse a scratch-buffer every time. 
- This buffer can be propagated internally, making all the internal formatters either write directly to it,
- OR, make  use of the `tmp := buf.AvailableBuffer()` in some cases, where a byte buffer "extra capacity" can be temporarily used. 
- The `slog` package  uses `Attr` by value. It makes sense to minimize operating on them, since iterating / collecting into a new slice, iterating again etc causes copy-on-heap. Better to operate on them only once. 
- If we want to do padding, it's better to copy from a constant `space`-buffer than to invoke `bytes.Repeat` every single time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwasinger jwasinger jwasinger approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@holiman @jwasinger