search

ohler55 / ojg

Optimized JSON for Go
MIT License
857 stars 49 forks source link

Should jp.Expr thread-safe? #81

Closed zzzzwc closed 2 years ago

zzzzwc commented 2 years ago

Thank you for this library that allows me to import json data smoothly. But I still have a problem that when using jsonpath similar to [?(@.key0 == "value0")] to extract json data concurrently, it will operate jp.Script.stack concurrently. is it to save memory to put rhe "s.stack" on the structure? In order to enable concurrent access, is it possible to put "s.stack" on the stack?

ohler55 commented 2 years ago

Each individual jp.Expr is not thread safe. The jp.Exprs are fairly light weight so the intent was that a new instance would be created if used in a different thread. There is a performance improvement by reusing the stack so the option for concurrent use should be to keep different instances of a jp.Expr for concurrent use. Maybe using a pool to reduce the use of Mutex and locking.

ohler55 commented 2 years ago

Maybe it is possible to have some kind of option to make jp.Expr thread safe versus not. I have to think about that.

ohler55 commented 2 years ago

Did you want to start a discussion on how this might become an option?

zzzzwc commented 2 years ago

umm🤔,I haven't read the code of your repo completely, but only in the scenario mentioned by issue, I think that putting s.stack on the structure has relatively little performance improvement, and "jsonpath" is like A rule that will not change once it is confirmed, and adding this option will also increase the user's burden, so if I write it, I will make jsonpath thread-safe by default.

ohler55 commented 2 years ago

I'll do some benchmarks.

ohler55 commented 2 years ago

Finally got around to running the benchmarks. With some care there is not additional overhead in creating a new stack each time and that does make the scripts thread safe. I'll make that change. Thanks for not giving up.

ohler55 commented 2 years ago

Fixed in v1.12.12 which was just tagged and released.