lower the number of allocations in broadcastwriter by unclejack · Pull Request #8041 · moby/moby

unclejack · 2014-09-15T16:41:12Z

This PR changes the JSONLog marshalling to use custom code for marshalling. This helps improve performance and to lower the amount of allocated memory.

initial to V1

benchmark                    old ns/op     new ns/op     delta       
BenchmarkBroadcastWriter     3274647       1165596       -64.41%     

benchmark                    old MB/s     new MB/s     speedup     
BenchmarkBroadcastWriter     7.56         21.23        2.81x       

benchmark                    old allocs     new allocs     delta       
BenchmarkBroadcastWriter     7550           2510           -66.75%     

benchmark                    old bytes     new bytes     delta       
BenchmarkBroadcastWriter     590654        96739         -83.62%

initial to V2

benchmark                    old ns/op     new ns/op     delta       
BenchmarkBroadcastWriter     3274647       1148698       -64.92%     

benchmark                    old MB/s     new MB/s     speedup     
BenchmarkBroadcastWriter     7.56         21.54        2.85x       

benchmark                    old allocs     new allocs     delta       
BenchmarkBroadcastWriter     7550           1510           -80.00%     

benchmark                    old bytes     new bytes     delta       
BenchmarkBroadcastWriter     590654        64739         -89.04%

V1 to V2

benchmark                    old ns/op     new ns/op     delta      
BenchmarkBroadcastWriter     1165596       1148698       -1.45%     

benchmark                    old MB/s     new MB/s     speedup     
BenchmarkBroadcastWriter     21.23        21.54        1.01x       

benchmark                    old allocs     new allocs     delta       
BenchmarkBroadcastWriter     2510           1510           -39.84%     

benchmark                    old bytes     new bytes     delta       
BenchmarkBroadcastWriter     96739         64739         -33.08%

Fixes #3040
Fixes #7757

tiborvass · 2014-09-15T16:51:27Z

The benchmarks are impressive but we need to find a way of managing the code generation if we think it's a good compromise using ffjson.

LK4D4 · 2014-09-15T17:30:40Z

Am I right that we need change this on every code regeneration? Could you pls provide benchmark comparison with "vanilla" ffjson code and modified?

tiborvass · 2014-09-15T18:52:06Z

tiborvass · 2014-09-15T18:56:06Z

I wonder whether this should rather be in a jsonutils package instead.

@tiborvass I guess it could be timeutils or jsonutils at this point. I'm not sure what other JSON or time related utils we'll add and that's what makes it difficult to decide at this point where we should add it.

I'm fine either way.

Aha, we can move utils.RFC3339NanoFixed here!

@LK4D4 It's done.

crosbymichael · 2014-09-15T19:17:15Z

I'm getting test failures:

--- FAIL: TestLogsTimestamps (0.45 seconds)
        docker_cli_logs_test.go:98: Expected log 101 lines, received 1308
[PASSED]: logs - separate stderr (without pseudo-tty)
[PASSED]: logs - stderr in stdout (with pseudo-tty)
--- FAIL: TestLogsTail (0.58 seconds)
        docker_cli_logs_test.go:200: Expected log 101 lines, received 1193

unclejack · 2014-09-15T20:09:59Z

@crosbymichael Thanks for checking it out! I've updated the PR to fix the problem.

Docker-DCO-1.1-Signed-off-by: Cristian Staretu <[email protected]> (github: unclejack)

LK4D4 · 2014-09-17T13:19:42Z

I believe you can use WriteByte to not allocate slice.

@LK4D4 I've made the change.

Docker-DCO-1.1-Signed-off-by: Cristian Staretu <[email protected]> (github: unclejack)

LK4D4 · 2014-09-17T14:07:47Z

Here is my results:

benchmark                    old ns/op     new ns/op     delta       
BenchmarkBroadcastWriter     5514971       2234615       -59.48%     

benchmark                    old MB/s     new MB/s     speedup     
BenchmarkBroadcastWriter     4.49         11.07        2.47x       

benchmark                    old allocs     new allocs     delta       
BenchmarkBroadcastWriter     7550           1510           -80.00%     

benchmark                    old bytes     new bytes     delta       
BenchmarkBroadcastWriter     590688        64765         -89.04%

LGTM

crosbymichael · 2014-09-17T22:26:37Z

LGTM

…iter lower the number of allocations in broadcastwriter

LK4D4 reviewed Sep 15, 2014
View reviewed changes

unclejack force-pushed the lower_allocations_broadcastwriter branch from c30c27f to 305b9c1 Compare September 15, 2014 18:47

tiborvass reviewed Sep 15, 2014
View reviewed changes

Comment thread pkg/jsonlog/jsonlog_marshalling.go Outdated

Copy link
Copy Markdown

Contributor

tiborvass Sep 15, 2014

Choose a reason for hiding this comment

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

BINDDIR

unclejack force-pushed the lower_allocations_broadcastwriter branch from 305b9c1 to df29f8a Compare September 15, 2014 18:54

tiborvass reviewed Sep 15, 2014
View reviewed changes

unclejack force-pushed the lower_allocations_broadcastwriter branch 2 times, most recently from dffce2e to d1214b7 Compare September 15, 2014 20:09

unclejack force-pushed the lower_allocations_broadcastwriter branch from d1214b7 to e5dee84 Compare September 16, 2014 22:02

add the timeutils package

9ae3134

Docker-DCO-1.1-Signed-off-by: Cristian Staretu <[email protected]> (github: unclejack)

LK4D4 reviewed Sep 17, 2014
View reviewed changes

use custom marshalling for JSONLog

9851e8c

Docker-DCO-1.1-Signed-off-by: Cristian Staretu <[email protected]> (github: unclejack)

unclejack force-pushed the lower_allocations_broadcastwriter branch from e5dee84 to 9851e8c Compare September 17, 2014 14:03

crosbymichael added a commit that referenced this pull request Sep 17, 2014

Merge pull request #8041 from unclejack/lower_allocations_broadcastwr…

77d30e7

…iter lower the number of allocations in broadcastwriter

crosbymichael merged commit 77d30e7 into moby:master Sep 17, 2014

unclejack deleted the lower_allocations_broadcastwriter branch September 17, 2014 22:51

Conversation

unclejack commented Sep 15, 2014

Uh oh!

tiborvass commented Sep 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Sep 15, 2014

Uh oh!

unclejack commented Sep 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Sep 17, 2014

Uh oh!

crosbymichael commented Sep 17, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants