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

Add initial Set implementation #61 #102

Merged
merged 2 commits into from
Jun 3, 2017
Merged

Add initial Set implementation #61 #102

merged 2 commits into from
Jun 3, 2017

Conversation

JoshuaC215
Copy link
Contributor

@JoshuaC215 JoshuaC215 commented May 11, 2017

Description:
This PR adds support for Set method, enabling end users to use jsonparser to generate json efficiently.
Note: I am taking this over from #98 after discussion with @ryanleary as he does not have cycles to complete it at the moment.
Benchmark before change:

BenchmarkJsonParserLarge-4                         50000            170465 ns/op               0 B/op          0 allocs/op
BenchmarkJsonParserMedium-4                       200000             29789 ns/op               0 B/op          0 allocs/op
BenchmarkJsonParserEachKeyManualMedium-4          500000             18119 ns/op             112 B/op          2 allocs/op
BenchmarkJsonParserEachKeyStructMedium-4          500000             21547 ns/op             656 B/op         13 allocs/op
BenchmarkJsonParserObjectEachStructMedium-4       200000             31427 ns/op             608 B/op         12 allocs/op
BenchmarkJsonParserSmall-4                       3000000              2414 ns/op               0 B/op          0 allocs/op
BenchmarkJsonParserEachKeyManualSmall-4          3000000              2058 ns/op              80 B/op          2 allocs/op
BenchmarkJsonParserEachKeyStructSmall-4          2000000              3052 ns/op             256 B/op          9 allocs/op
BenchmarkJsonParserObjectEachStructSmall-4       3000000              2545 ns/op             240 B/op          8 allocs/op

Benchmark after change:

BenchmarkJsonParserLarge-4                         50000            147591 ns/op               0 B/op          0 allocs/op
BenchmarkJsonParserMedium-4                       300000             28021 ns/op               0 B/op          0 allocs/op
BenchmarkJsonParserEachKeyManualMedium-4          500000             18563 ns/op             112 B/op          2 allocs/op
BenchmarkJsonParserEachKeyStructMedium-4          500000             19804 ns/op             656 B/op         13 allocs/op
BenchmarkJsonParserObjectEachStructMedium-4       300000             29924 ns/op             608 B/op         12 allocs/op
BenchmarkJsonParserSmall-4                       3000000              2089 ns/op               0 B/op          0 allocs/op
BenchmarkJsonParserEachKeyManualSmall-4          3000000              2169 ns/op              80 B/op          2 allocs/op
BenchmarkJsonParserEachKeyStructSmall-4          2000000              3138 ns/op             256 B/op          9 allocs/op
BenchmarkJsonParserObjectEachStructSmall-4       3000000              2822 ns/op             240 B/op          8 allocs/op
BenchmarkJsonParserSetSmall-4                    2000000              4086 ns/op             816 B/op          5 allocs/op

Also note compare last to comparable SetPath() benchmark in go-simplejson:

BenchmarkGoSimplejsonSetSmall-4           500000             16755 ns/op            2289 B/op         40 allocs/op

@JoshuaC215
Copy link
Contributor Author

@bruth I added fixes and tests for the edge cases you mentioned in the other PR, and a couple of others (e.g. trailing whitespace). Also added a couple simple benchmarks. Note sure about the empty byte slice on error but I notice it is mentioned specifically in the documentation.

Let me know if anyone sees other issues, has thoughts or requests for other things to add or change. It has the same limitations about not catching malformed JSON as the underlying Get() method, and I did tag it as experimental in the README.

@JoshuaC215
Copy link
Contributor Author

FYI @buger we are in process of integrating jsonparser with Set() (and #103) into our tool kazaam, definitely putting it through its paces and finding it much improved over simplejson we were using before. qntfy/kazaam#35

@JoshuaC215
Copy link
Contributor Author

Fixed a bug where Set() in existing empty object would produce an extra comma

        {
                desc:    "set unknown key (object, empty partial subtree exists)",
                json:    `{"test":{}}`,
                isFound: true,
                path:    []string{"test", "new.field"},
                setData: `{"key":"new object"}`,
                data:    `{"test":{"new.field":{"key":"new object"}}}`,
        },

@buger
Copy link
Owner

buger commented Jun 3, 2017

Sorry for the delay!
This is really cool!

@buger buger merged commit bb14bb6 into buger:master Jun 3, 2017
@JoshuaC215 JoshuaC215 deleted the add-set-impl branch June 4, 2017 20:31
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 this pull request may close these issues.

2 participants