-
Notifications
You must be signed in to change notification settings - Fork 25
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
Slower than JSON.parse #28
Comments
Hi @dalisoft, Thank you so much for making your tests available to me so I could save some time. As you mentioned, I changed the parameters of the code you asked me to test. Instead of a For all three functions of
|
Oh, here is the usage of
See that it does not construct the actual JS object, it keeps an external pointer to the C++ buffer and for this reason, you can only access the keys with the |
It is generally a challenging task but simdjson should still be faster than the competition. |
The simdjson library itself is faster even on small files. |
Is i'm doing something wrong or at cost of bindings performance isn't what i want. const simdjson = require('simdjson');
const jsonString = "{ \"foo\": { \"bar\": [ 0, 42 ] } }";
const JSONbuffer = simdjson.lazyParse(jsonString); // external (C++) parsed JSON object
console.log(JSONbuffer.valueForKeyPath("foo.bar[1]")); // 42 I see it's good, but i'm using for other case const simdjson = require('simdjson');
// some code
// all below code is repeated a lot of times
const JSONbuffer = simdjson.lazyParse(req.body); // req.body - JSON string
console.log(JSONbuffer.valueForKeyPath("")); // To get all object I want use this library within my backend framework for node.js as |
Hi @dalisoft, I see your point now. There are only a few cases where you actually need the whole json, but for the root case, it should have the same performance for this wrapper. I will think of new approaches to improve the library but will only be able to do it in the future. I also will take a close look to the repo https://github.com/croteaucarine/simdjson_node_objectwrap @croteaucarine. She’s working on improvements for the wrapper. I will leave this issue open until we fix it. Hopefully it won’t take (that) long. Cheers! |
@luizperes Thanks, i'll wait :) |
Note to self: there are a few leads on PR #33 |
@luizperes did you consider the way node-expat has chosen? |
@xamgore can you elaborate your question? |
@luizperes Hi |
@luizperes with node-expat you can add js callbacks for events like "opening tag", "new attribute with name x", etc, so only the required properties are picked, copied and passed back to the javascript thread. It's a contrary to the method of a smart proxy object, and still doesn't require a big amount of data to be passed between the addon and v8. |
So just to confirm, if you want to get the entire object from a string(eg lazy usage isn't possible), this probably isn't the library to use in it's current state? |
@RichardWright I cannot speak specifically for this library but one general concern is that constructing the full JSON representation in JavaScript, with all the objects, strings, arrays... is remarkably expensive. In some respect, that's independent from JSON. cc @jkeiser |
Passing around a buffer and using key access is the preferred method then? |
That is correct @RichardWright. My idea, as mentioned in other threads, would be to have simdjson implemented as the native json parser directly into the engine e.g V8. That would possibly speed up the process. At this moment I am finishing my masters thesis and don't have time to try it, so we will have to wait a little bit on that. :) |
CC @mcollina |
@luizperes cool, thanks for the response! |
Hi @luizperes
I know this library was made to handle large JSON files, but there i occurred to some performance stranges when tried to parse my json and benchmarked, this library is slow, up to 6-x slower.
Here result:
Also,
lazyParse
does not return expected result for me or i'm doing something wrong, and even using lazyParse, performance still slow. How we can improve this?Code to test
The text was updated successfully, but these errors were encountered: