optimized reserve call for memory allocation in advance #15479
Conversation
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
| { | ||
| // Reserve space for efficiency | ||
| size_t numExpressions = _expressionsToEvaluate.size(); | ||
| command.reserve(command.size() + numExpressions * 100); // Estimate space needed |
There was a problem hiding this comment.
Where did you come up with the 100 chars per expression heuristic? Also, you're not using shrink_to_fit() before returning the command, so you're most likely returning a much larger command string than you should be.
@blishko I'd be inclined towards closing this is a mostly unnecessary optimization, but it's up to you.
There was a problem hiding this comment.
I agree that this is unnecessary optimisation, with a magic number involved.
If anything, command should be a stringstream. That would also achieve the desired result in a clearer way.
But this is definitely not any bottleneck, AFAIK.
There was a problem hiding this comment.
Caching of vector's size() is unnecessary as well. It's guaranteed O(1) and introducing an extra local variable for something so trivial is a hit to readability (it's another name you have to keep track of in your head when reading the code) so not a good trade-off.
I'd not be against us using things like reserve() more in places where the size is non-trivial and known ahead of time, but TBH most of the time the impact is probably insignificant so it does not matter all that much in the big picture. Just changing it randomly in a single spot that is not a known bottleneck is pointless.
|
Given the current sentiment regarding this PR, I'll close it as 'won't do'. Regardless of this not being merged, thanks for contributing @blacroc10! |
|
@blacroc10 Sorry, but this is the kind of contribution we're generally not interested in. The PR is just a random micro-optimization for the sake of it. It's also very sloppy (e.g. it completely disregards the style of the surrounding code by rewriting randomly replacing all indents in the middle of a file). |
may help reduce re allocations during concatenation