14:29:56 Hi all, I've previously asked about replacing the JSON serialization in epee with a nicer library. There were some valid questions regarding the perceived benefits and potential slowdown, so I've sunk some time into testing this out. You can see my results here: https://github.com/bertptrs/json-speedtest 14:30:25 In short: everything I've tested compiles faster, results in a smaller binary, and runs faster 14:31:35 Given this information, would you be willing to accept a PR that replaces the current serialization code (in the wallet-rpc etc) with something like json_dto for example? 14:32:58 this is good work 14:35:18 It seems odd json_dto is faster than rapidjson, yet is said to be a wrapper around it. 14:36:17 awesome work bertptrs! 14:37:20 That'd be the KV_SERIALIZE system only, right ? 14:38:12 (or also the FIELD one, which can do both json and binary) 14:39:13 Anyway, if those timings are fair and there are no hidden strings, they're enough for me to agree with the replacement. 14:41:36 moneromooo: rapidjson doesn't really have any deserialization primitives, so if you look at the code, you'll see that it's very ad-hoc. Most of the difference in speed between json_dto and rapidjson is due to small optimizations, such as reserve()ing the correct size for a vector 14:41:59 This would indeed just be for the KV_SERIALIZE system 14:43:46 vtnerd: ^ 14:45:54 BTW, we do have some invalid JSON in so called binary calls, eg getblocks.bin. 14:46:48 The intent is extra speed. It'd be nice to look at whether the bin/notbin distinction still makes sense with json_dto. 14:47:05 I don't think I've ever checked how much speed is gained by this. 14:54:49 The only real reason why that could improve performance is because it reduces the amount of network data used; there are ridiculously fast (en|de)coders for e.g. base64 14:57:20 moneromooo: for hidden strings: feel free to browse the test code, it's only ~300LOC, and most of them are trivial. For invalid JSON: that's a bit unfortunate, but I'd need to look into it 15:28:40 Could you give me some direction on how I could best compare the bin/not bin functions? AFAICT there's just 6 (or 9, if you count the aliases) of them, so I'm hoping it's not too much trouble 15:33:55 Compare ? If you mean test them, then they're called from wallet2. 15:34:07 ie, refresh, and getblocks.bin gets called. 15:34:26 The difference is KV_SERIALIZE_AS_BLOB (or similar name). 15:34:53 The "blob" means the vector is memcpied as is, rather than go through json dump/parse. 15:43:28 Ah, I see. That might be faster in some cases, question is of course how much 19:04:58 I have no idea why we would use json_dto - we can do the same thing without that library 19:05:32 it uses rapidjson internally, we might as we switch the epee json from a custom `std::map` to rapidjson, which is most of the speed is probably coming from 19:07:00 and json_dto is going to be slower than the work I just for zmq-json (which unfortunately has always been separate from epee-json stuff) because it writes to a rapidjson DOM first instead of writing directly to a stream 19:09:02 and fwiw, I can get something like an additional 3x speedup (from that last PR) in zmq-json output by using the json output used by blocks/txes (which is separate from epee! oh lord) .... but the patch required a bit more custom work 19:09:31 json_dto is going to b slower-> json_dto output is going to be slower 19:15:18 as for bin/not bin ... the time spent in hex is decent based on my timing profile I did. I looked at the ASM for the function in use, and theres nothing obvious that can be done 19:16:44 I re-worked the zmq stuff some more - I have some ideas on how to get something like epee but with proper .h/.cpp and support msgpack. A little worried about the advanced macro usage though 19:17:40 in summary, bertptrs if you had something based around rapidson for epee then it might be worth it, but Im still not seeing json_dto usage 19:17:55 vtnerd fluffypony only fluffy has access to the supercop repo easily I think 19:18:10 hmm, ok 19:20:03 and unfortunately the next PR is hell. Theres a decent chunk of ASM to maintain constant-time and batched field inversions on the ECDH. But hey moo, its not C++ lol 19:20:48 its 95% identical to existing ASM in the library, so that should help with the review at least 19:21:36 new supercop PR incoming? 19:21:38 :D 19:21:41 anyway luigi1111w -> should that repo be distributed on write access? I wouldn't expect frequent changes 19:22:22 yes, I had a really nice benchmark utility and everything thats just sitting here on my drives which is absurd 19:24:43 this will differ from the ryo implementation because its using C++ function aliases (`using generate_key_derivation`, etc.)to do compile-time binding, so theres no runtime overhead if using the default portable monero library 19:25:35 If we can get most of the speedup by changing small things, I'm all for it. 19:25:56 we just have to ping fluffypony fluffypony fluffypony until he shows up to grant access 19:26:28 I can ping him on wire but I used my weekly ping there already 19:26:29 :D 19:34:01 moneromooo : json_dto is doing what the epee/serialization folder is doing or what the src/serialization/json_object.h (zmq-json stuff) is doing. 19:35:35 more specifically its what src/serialization/json_object.h is doing - its trying to provide some utilities for mapping C++ to rapidjson 19:37:30 thats kind of why I'm negative on it ... we already have something similar (in another part of the code :/), and I just did a PR that accelerates its output further but not wasting cycles on creating a DOM (which json_dto does) 19:40:23 Odd. I had kinda assumed the speedup was from not creating a DOM :) 19:41:07 luigi1111w: done 19:41:25 see, you just have to say my name 3 times 19:41:41 rapidjson DOM should be more performant than the one in epee, but I've never clocked it do know the difference 19:42:07 well the secret is out now, all hell will break loose 19:43:14 bertptrs: how do you feel about checking a bastard of epee and rapidjson as vtnerd is suggesting ? :) 19:43:36 Not having to move the loads of KV_SERIALIZE would be nice. 19:44:21 the genie has arrived 19:45:03 the serialize functions all take a templated storage type, so starting with a copy of `epee::portable_storage` is what you'd want. I'm not sure how much hell its going to be with the function signatures though 19:45:45 When I tried, it pulled lots (that patch you did not like due to the std::string needed for find). 19:46:20 Well, I was modifying the class, not replacing it I guess. 19:46:38 if you were blowing up the epee stuff entirely, then we might as well leverage `src/serialization/json_object.h`, unless there's a good reason not to use it 19:47:12 *yes I realize re-doing KV_SERIALIZE is not ideal, I'm saying why blow it for json_dto when we got nearly the same thing already 19:48:28 moo : yup, there might be some funky things when working with teh function signatures. as an example, the epee thing isn't doing `reserve` anywhere due to its design 19:49:06 there should be a way to work that in, but requires getting a headache first 19:49:17 <3 rapidjson