Skip to content
This repository was archived by the owner on Dec 4, 2019. It is now read-only.

[prototype] Add type-specific decoder code generator - #26

Open
tsholmes wants to merge 4 commits into
masterfrom
add/generate
Open

tsholmes wants to merge 4 commits into
masterfrom
add/generate

Conversation

@tsholmes

Copy link
Copy Markdown

This adds a new objconvgen entrypoint that generates a optimized decoder for a specific type. Since the decoder doesn't need to use reflection and can return all results by-value, it attains a significant speedup from the generic parsing.

Since this is a prototype, I'd like to get some feedback on the approach I took. Any and all suggestions and feedback are welcome. This is a pretty big feature, so I'd like everyone to be on board with it.

Sample benchmark on a structure with strings, times, and nested arrays

BenchmarkDecodeCBOR-8                  200000          6431 ns/op          80 B/op           6 allocs/op
BenchmarkDecodeJSON-8                  200000          8607 ns/op          80 B/op           6 allocs/op
BenchmarkDecodeCBORGenerated-8         500000          3319 ns/op          48 B/op           5 allocs/op
BenchmarkDecodeJSONGenerated-8         300000          5425 ns/op          48 B/op           5 allocs/op

What's supported right now:

  • structures
  • arrays (not slices)
  • primitives
  • time.Time
  • aliased types
  • types in different packages

What's not supported right now:

  • slices
  • maps
  • time.Duration
  • []byte/[N]byte special case

What definitely needs to be done before this could be merged:

  • support json and objconv struct tags
  • write the output to a file and goimports/gofmt it
  • add tests on some generated code
  • add benchmarks on some generated code

What would be nice to clean up before this is merged:

  • remove dependence of generated code on github.com/segmentio/objconv/generate/util package

@achille-roussel achille-roussel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking great overall 👍

One last comment, what do you think about using a sub-command of objconv for the CLI? (objconv generate)?

I can see how a syntax like this could be convenient as well:

objconv generate github.com/segmentio/pkg.Type

It can output to stdout by default, then keep the overwrite with the -o option you have right now.

Comment thread generate/util/util.go
v = int64(uv)
case objconv.Float:
var fv float64
fv, err = p.ParseFloat()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we check that the fractional part is zero as well?

This could help => https://golang.org/pkg/math/#Modf

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah that's what the double cast was for: float64(int64(fv)) != fv. this also catches very large numbers that get clobbered on a cast to int64

Comment thread generate/util/util.go Outdated
@@ -0,0 +1,235 @@
package util

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about naming the package genutil? Seems like it could communicate a little better what it's intended to be used for?

Could be a good idea to add a doc.go with a package documentation stating that the functions are here to support generated code and should not be accessed directly.

Comment thread generate/generate.go Outdated
}

func (d *%[3]s) Decode() (%[4]s, error) {
return d.decode_%[5]s()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is using a _ required here? golint usually complains about that.

Comment thread generate/generate.go
return walkType(typeObj.Type()), nil
}

func GenerateDecode(path string, typ string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about having a signature based on io.Writer? Maybe something like this:

func WriteDecoder(w io.Writer, path, typ string) error

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants