Store parser directly instead of extracting document #36
Conversation
|
Hi @jkeiser, I checked out your branch (locally) and the performance gets a little worse than before. Here is what the first item of the benchmark looks like: with
|
|
Yeah, it depends what file you're using. Some get worse, some get better. It actually might be real, and I have no explanation. I ran a few more times and threw out the highest results for each, and got this: This PR
master
|
|
Regardless, this is what simdjson_nodejs used to do, and it is the only way to avoid std::move. I don't recommend checking it in unless we see a more consistent win from it; I really think we want to be stealing the document from simdjson, at least to reduce memory pressure. |
|
I agree. Won't check it in for now. What is discussed on #35 might help this approach, since the cost now seems to be related to allocating a new parser. |
@luizperes where does this come from, BTW? Did simdjson_nodejs used to go this fast? I went back and looked; the old code did std::move on the parser, which this change doesn't even do: ParsedJson *pjh = new ParsedJson(std::move(pj));
Napi::External<ParsedJson> buffer = Napi::External<ParsedJson>::New(env, pjh,
[](Napi::Env /*env*/, ParsedJson * data) {
delete data;
});I know I must seem like some kind of std::move partisan here, I'm really not. I just suspect we're jumping at it because it's the most obscure thing in the code (which it definitely is!) :) |
@jkeiser I know it can (possibly) go that fast (and maybe even a little faster than that, will try to explain below). I got that value from the code below (using simdjson_nodejs has three methods as I think you know (documented here): Also, you see that, up to here, isValid
Keeping a reference should be very fast, but as there is some extra work (of course), I believe that the cost of doing a Let me know if you see something wrong in my explanation! :) |
I see what you're saying. I don't know that we've made a small enough change to say it's the std::move though. There are big things the compiler could do if it figures out that all Externaldom::document instances have identical documents with nullptrs in them. In particular, it could potentially compile the destructor down to nothing. My supposition to this point has been that attaching a destructor function to Napi::External<> is the thing that makes things slow (a lot of GC languages optimize the crap out of things with no destructors). I think to compare apples to apples we need to actually fill in the document. I pushed up a jkeiser/std-move-experiment branch of simdjson_nodejs here that removes std::move(). It also makes clear what I believe std::move is doing under the covers by removing unique_ptr. Now the document is just two plain old pointers, and construction copies the pointers and nulls the original document's pointers. Might be a good starting point for experimentation, at least. I don't see a significant difference between this and master. Maybe you will, though! For fun, you can also go back one commit on that branch, and see what difference std::move makes
Be careful here :) The compiler is quite capable of deleting huge amounts of code if it can figure out that you're not using its results. I once thought I'd doubled simdjson's speed with a change of mine, until I fixed a bug: I had forgotten to check the utf-8 validation's I can't say that's what's happening here. But I can say that there's a lot of stuff the compiler could do given that it knows no one else could possibly be using the results of that parse. |
That is correct. It could be that after the compiler optimizations they were computationally equivalent codes. I will take a look at your experiments, thanks a lot! |
|
FYI @jkeiser, when I do something like: I get: (Not a |
|
As for your branch |
|
Complementing my question, it is just that copying by reference would be much faster than copying by value. I think that keeping |
We're talking about doing something like that. It's worth seeing what the performance is, at least! Note: you shouldn't really make changes to the simdjson.h and simdjson.cpp you have here, except if you're just experimenting like this.
Yes, it's copying by value. a reference copy would be a single word write. This involves a 2-word malloc and 4 word writes (2 of them to null out the old pointer). Copying by reference will absolutely be faster. However, it seems really, REALLY unlikely that a 2-word malloc and 3 stores are dominating the runtime, especially given that they happen exactly once per parse. It seems more likely to be a caching or inlining effect. But hypotheses are cheap :) It will come down to experimenting and measuring until it's nailed down.
Yep. Again, this could either be because of the 2-word malloc and 3 extra word stores, or an effect on inlining / cache since the pointers now live in two different places over the life of the document, or something else we haven't thought of. |
|
I'm going to close this so it doesn't accidentally get merged, but am happy to continue talking about it :) Note I haven't forgotten about bindings, but I plan to focus on making streaming parsing work for a little bit before I come back to it :) |
|
Sounds good, thank you @jkeiser |
This gets rid of the std::move(document) by just storing the parser. It obviously requires the parser to be
new'd up. It definitely doesn't make up the difference and it's even pretty hard to tell if it helps--there's some things better and some things worse. But it could at least eliminate that as a source of confusion.I'd love to see if other folks get more reproducible results out of this :/
This PR
master