Optimize memory allocations in VariantParser::get_token

[MednauN]

This is optimization I made after profiling loading time of a game we're making. The game has lots of heavy scenes with meshes, animations and stuff, so parsing them became a bottleneck.
Apparently, there are a lot of redundant memory allocations inside VariantParser::get_token. I rewrote the code so it will use static buffer when possible.

Just a couple of screenshots - this is profiling results of loading phase of our game:

And this is the same loading after my optimization:

And I'm aware that it's only actual for development builds (exported projects are unlikely to have this issue at all). Still, I think it won't hurt to merge.

[hpvb]

I like the idea for the optimization but it seems to me it would be better to decouple a String's length() from the underlaying array.size(). That way everything benefits from a small performance win due to less allocation.

It seems that keeping the string's length separately from it's size wouldn't be terribly hard to do. Once that's done you could just resize num to start off with a reasonable number of bytes.

I don't think this should be merged in this form personally.

[MednauN]

@hpvb you certainly have a point here, but separating string length from array size will cause String::length and String::size return completely different values, and I'm afraid that may break something in existing code.
And the static buffer is also important here, because it saves us from any memory allocations in most of cases. (actually, it's similar how std::string works, and I really wanted to add this optimization to String itself, but since it's inherited from Vector<CharType>, I can't do this)
Hm... or maybe, "static buffer" thing may be discarded here without much of performance loss. Gonna check this with profiler.

[MednauN]

This is what happened when I left only long_num_buffer (initial size 64 bytes, growth strategy 1.5):

It was tested on a different scene, so total count of samples is different, sorry about that.
But performance relative to get_token should be pretty accurate, so I assume that performance drops more than twice because of Vector::resize and Vector::~Vector.
So, as I said, static buffer is important.

[hpvb]

My concern is that there are tons of places in godot where this type of parsing happens. Maybe add a new helper container that implements this algorithm and replace all of these types of rapid-fire appending cases in parsers with that.

I just really don't like this mini-string implementation in the middle of a parser.

[MednauN]

@hpvb oh, yeah, a helper container sounds like a good idea. Thanks for the hint. I think I'll make something like StringBuilder from Java if it's okay.

[hpvb]

@MednauN That sounds about perfect! 👍

[karroffel]

num_buffer_size needs to be a static const int in order to comply to the C++ standard. The way it is now is probably accepted by most compilers because they can detect it's a constant expression, but by the standard this is not a valid declaration.

With -std=gnu++03 this might work, but it's again not C++ standard conform.

I mean it will most likely work on all compilers currently used, just pointing it out here. 😄

[karroffel]

@MednauN hey, would you mind if I work on that StringBuilder class? A PR should be ready in like an hour.

[MednauN]

@karroffel okay, suit yourself)

[karroffel]

@MednauN #10860 here you go!

[MednauN]

@karroffel well, that's one way to make it, but I'm afraid, in context of this PR the StringBuffer will be lacking in performance. As I wrote before, using static buffer (i.e. fixed-size array of chars) is one of important points of the whole optimization thing here. Thus, if I want to re-write PR using the StringBuffer, I would need to improve it so it'll use a static buffer before allocating memory.
Well, sure I can add these changes myself, but since you're already writing the StringBuffer implementation, maybe you should consider to add this optimization as well.

[karroffel]

@MednauN I wanted to implement that in the past, so to be quite honest, I didn't make it for your PR, I just took the chance because this time reduz understood what I actually meant with a StringBuilder 😄

If you could comment that on the PR that would be cool. I could implement a static-sized member array that's used before any other dynamic allocation, that would be doable, but I fear that reduz will have something against it. But I would appreciate it if you could make a list of demands in the PR.

[MednauN]

I made StringBuffer that does all my optimizations. Personally I think it should be part of StringBuilder from #10860, but since karroffel says that it would be better to separate them, so be it.

[reduz]

This is cool, but just remember that (even though it's not working yet) Godot will convert your text scenes to binary eventually on export.

…

On Mon, Sep 4, 2017 at 12:57 PM, MednauN ***@***.***> wrote: I made StringBuffer that does all my optimizations. Personally I think it should be part of StringBuilder from #10860 <#10860>, but since karroffel says that it would be better to separate them, so be it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#10844 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z26ySmUkXw4vGhsm0o_2sMIkrDjyEks5sfB3TgaJpZM4PJt2o> .

[reduz]

Well, it does on 2.1, it still doesn't on 3.0 as the system for remapping changed.

…

On Mon, Sep 4, 2017 at 1:11 PM, Juan Linietsky ***@***.***> wrote: This is cool, but just remember that (even though it's not working yet) Godot will convert your text scenes to binary eventually on export. On Mon, Sep 4, 2017 at 12:57 PM, MednauN ***@***.***> wrote: > I made StringBuffer that does all my optimizations. Personally I think > it should be part of StringBuilder from #10860 > <#10860>, but since karroffel > says that it would be better to separate them, so be it. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#10844 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AF-Z26ySmUkXw4vGhsm0o_2sMIkrDjyEks5sfB3TgaJpZM4PJt2o> > . >

[akien-mga]

There are small code style issues that need to be fixed: https://travis-ci.org/godotengine/godot/jobs/271743463

While at it, I think it might make sense to split your changes in two commits for clarity: first add StringBuffer, then use it in the second commit.