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

Ultra-fast, eminently lossy NullEncoder #133

Merged
merged 1 commit into from
Aug 29, 2016
Merged

Conversation

billf
Copy link
Contributor

@billf billf commented Aug 18, 2016

No description provided.

// nullEncoder is an Encoder implementation that throws everything away.
type nullEncoder struct{}

// NewNullEncoder creates a fast, no-allocation encoder.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since there's no options or any customizations for this encoder, does it make sense to just have a single global instance and remove the NewNullEncoder constructor?

var NullEncoder Encoder

func init() {
  NullEncoder = nullEncoder{}
}

(I like to separate the declaration from the assignment so that the = nullEncoder{} doesn't show up in the godoc)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that approach too. One thing to consider is that it'll make the GoDoc output a little less nice - with a constructor function, all the different encoder constructors are grouped under the Encoder type. Top-level vars are in a completely different section.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, maybe just func NullEncoder() Encoder then? It'll still be backed by a global by it will show under the Encoder type?

@prashantv
Copy link
Collaborator

My new favourite encoder (after all, it's the fastest, and has the least code).

Can we add unit tests to avoid the coverage drop (I can't imagine the tests being too exciting unfortunately, but at least we can make sure that nothing was written to the writer no matter which method is called).

I'm also curious what the benchmarks are! This will give us numbers on just the function call / arg passing overhead compared to the actual work we do in our real encoders.

@billf
Copy link
Contributor Author

billf commented Aug 22, 2016

BenchmarkStandardJSON-8                  1000000              2647 ns/op
BenchmarkZapJSON-8                      10000000               254 ns/op
BenchmarkZapNull-8                      100000000               14.5 ns/op
BenchmarkJSONLogMarshalerFunc-8         10000000               122 ns/op
BenchmarkNullLogMarshalerFunc-8         30000000                43.2 ns/op

func (nullEncoder) AddMarshaler(_ string, _ LogMarshaler) error { return nil }
func (nullEncoder) AddObject(_ string, _ interface{}) error { return nil }

// Clone copies the current encoder, including any data already encoded.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can drop comment here too, since it doesn't actually copy the encoded data etc

@prashantv
Copy link
Collaborator

lgtm, thanks for adding the benchmarks. BenchmarkNullLogMarshalerFunc is more expensive than I expected, it looks like all it does is:

  • get a reference to an existing global null encoder
  • call 2 functions which do nothing, but through interfaces (so there's some vtable overhead)

However, since we're using LogMarshalerFunc, it allocates a closure to capture the reference to i. That allocation ends up taking most of the time.

@billf
Copy link
Contributor Author

billf commented Aug 24, 2016

should we pull LogMarshalerFunc out of both the json and null encoder and instead use a struct implementing LogMarshaler allocated outside of the benchmark loop?

since LogMarshalerFunc's implementation cannot ever optimized further, i don't see a reason to benchmark it after such a removal.

@prashantv
Copy link
Collaborator

Let's merge this as is, and we can move out the LogMarshalerFunc benchmark separately.

@prashantv prashantv merged commit fc9fbdf into uber-go:master Aug 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants