jsonparser icon indicating copy to clipboard operation
jsonparser copied to clipboard

EachKey panics if more than 64 keys in path

Open nieksand opened this issue 9 years ago • 6 comments

I'm dealing with a large, flat json payload that has 100+ keys in a map.

I hit panics when using EachKey with the full list of json keys:

goroutine 1 [running]:
panic(0x4e8ac0, 0xc42000a0f0)
    /home/niek/go17/src/runtime/panic.go:500 +0x1a1
github.com/buger/jsonparser.EachKey(0xc421480000, 0x11ac, 0x11ac, 0xc420050e30, 0xc4200da000, 0xaf, 0x100, 0x0)
    /home/niek/workspace/src/github.com/buger/jsonparser/parser.go:237 +0x6a6
  ...snipped...

The problem code seems here: https://github.com/buger/jsonparser/blob/master/parser.go#L236

If I'm reading it right, there is a limit of 64 keys per EachKey lookup before the int64 bitmask overflows.

Limit should probably be documented and code should return an error rather than panic.

nieksand avatar Jul 20 '16 00:07 nieksand

Correction.. only supports 63 keys given signed int64 the way bitwiseFlags is populated.

nieksand avatar Jul 20 '16 04:07 nieksand

Yes it is, I have few ideas how to fix it without sacrificing performance.

On Wednesday, 20 July 2016, Niek Sanders [email protected] wrote:

Correction.. only supports 63 keys given signed int64 the way bitwiseFlags is populated.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/buger/jsonparser/issues/56#issuecomment-233838352, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA2uZbxpBvOxfZp3-ahA6iCyfMDhHoFks5qXar-gaJpZM4JQTF9 .


Sincerely yours Leonid Bugaev http://gortool.com - test your system with real data

buger avatar Jul 20 '16 05:07 buger

First off, thank you for writing and sharing this library! It's been incredibly helpful for a little project I'm working on.

A wishlist item that relates to the many key lookup issue...

I wish there was a way to iterate over a map and just have a callback invoked for each key/value pair:

func(key []byte, value []byte, dataType ValueType, err error)

For a many-key scenario like mine, I can just lookup the key in a Go map to see if it's interesting to me.

Similarly, if you just want to chomp through all the keys in the map, this avoids having to determine/specify the keys you care about up front.

nieksand avatar Jul 21 '16 02:07 nieksand

I did a super-trashy hack implementation of my wishlist item above.

Before I invoked EachKey ~3 times per json payload to pull my data. (63 keys per call).

Now I use "WalkMap" once.

Overall performance of my app, which includes many things unrelated to json, went from 20K msgs/sec to 30k msgs/sec.

Before I was cpu-bound on json processing. Now I have free cpu and am bottlenecked elsewhere in my app.

Like I mentioned, the code is hacked garbage hence no PR.

But if you are curious it sits here: https://github.com/nieksand/jsonparser/blob/master/parser.go#L334

The actual change is here: https://github.com/nieksand/jsonparser/blob/master/parser.go#L365

nieksand avatar Jul 21 '16 05:07 nieksand

I've been thinking of prototyping this feature, as well. If I get some free time over the next couple days I may look at your code and spin something off of it for an actual PR.

On Jul 21, 2016 01:08, "Niek Sanders" [email protected] wrote:

I did a super-trashy hack implementation of my wishlist item above.

Before I invoked EachKey ~3 times per json payload to pull my data. (63 keys per call).

Now I use "WalkMap" once.

Overall performance of my app, which includes many things unrelated to json, went from 20K msgs/sec to 30k msgs/sec.

Before I was cpu-bound on json processing. Now I have free cpu and am bottlenecked elsewhere in my app.

Like I mentioned, the code is hacked garbage hence no PR. But if you are curious it sits here: https://github.com/nieksand/jsonparser/blob/master/parser.go#L334

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/buger/jsonparser/issues/56#issuecomment-234158152, or mute the thread https://github.com/notifications/unsubscribe-auth/ADvi_inTy6fvq0eF6D2AKhQ1xZb8G2BBks5qXv7qgaJpZM4JQTF9 .

daboyuka avatar Jul 21 '16 05:07 daboyuka

Very cool.

I'm thinking it might be possible to rewrite EachKey to run on top of WalkMap. That would avoid the code duplication issue.

nieksand avatar Jul 21 '16 05:07 nieksand