Skip to content
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

Avoid reallocation of vector using a list of nearby bodies inside Bod… #4546

Merged
merged 3 commits into from Mar 4, 2019

Conversation

Projects
None yet
3 participants
@mike-f1
Copy link
Contributor

commented Feb 24, 2019

…yNearFinder

Relevant changes are in Space.h and in Space.cpp,
remaining changes are to use the new prototypes
of GetBodiesMaybeNear

NOTE: Feel free to add comments & commits, because I feel
I'm not the best one at this...

@mike-f1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

@fluffyfreak

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

It's probably fine but not been reviewed as I've been out of the country for a week without a computer.

Reviews only happen when people have time to do so, that might mean only doing them every weekend (7 days), or even every other weekend (14 days) due to real life.

Please be patient ;)

@jaj22

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

There's still one allocation per GetBodiesMaybeNear call due to the vector copy return, right? As vector::clear doesn't actually reduce the capacity of a vector (C++11 weirdness), you could get a performance reduction for the UpdateAlertState usage. Return by reference plus lifespan comment should be fine.

Otherwise this seems like a general improvement.

@mike-f1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

There's still one allocation per GetBodiesMaybeNear call due to the vector copy return, right?

Actually return is by ref (see line 81 in Space.h: std::vector<Body *> &) so I can avoid moving copying and so on, but yes: the purpose of this PR was to reduce memory reallocations, then if doing it by move one allocation for each call (must?) be fine. So move could be retained as an option...
I colud add that actual drawbacks are that

  • compilers (at least mine: gcc) don't stop with an error if someone push pop or either free the container received but only give warnings and that

  • that list behaves as it is a "static" so it is "freed" only in ~Space (...when hyperjumping or when game exits...)

I'm fine also with moving that list (explicitly: like with a std::move in funtion return) as it is safer... Let me know your opinion.
A note: I speak of move and not copy: it will perform better because move should 'steals' the
memory reserved by m_nearBodies, thus decreasing its capacity to 0 (...but I didn't any checks
this way...)

As vector::clear doesn't actually reduce the capacity of a vector (C++11 weirdness)

I wanted to trade that reduction with less memory reallocations, because
I don't see cons in having that list only growing between ~Space calls.

you could get a performance reduction for the UpdateAlertState usage

I didn't get the reason, please: could you better explain your concern?

Otherwise this seems like a general improvement.

🙏

General OT

Return by reference plus lifespan comment should be fine.

Well, I hope you understand my thoughts: it is not I do not want
to comment code, it is that I don't think to be the one's that
may|should|could|must do that thing, expecially here where
comments could stay for long and there's a lot of stuff that needs
to be explained (...really a "lifespan comment" may suffice?).

Greetings

@mike-f1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

...really a "lifespan comment" may suffice?

...Indeed: how MP will behave?
(...I think in that case we should pass to "move" or a processor
could have its ref changed while working on it...)

@jaj22

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

I didn't spot that you'd changed the typedef of BodyNearList. The functionality here is fine but I'm really not a fan of hiding a reference in a typedef.

I wouldn't worry about MP. I suspect this code is low on the list of threading candidates even before this optimisation.

@mike-f1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

I didn't spot that you'd changed the typedef of BodyNearList. The functionality here is fine but I'm really not a fan of hiding a reference in a typedef.

Done...

I wouldn't worry about MP. I suspect this code is low on the list of threading candidates even before this optimisation.

...but it may worth a mention in a comment (*)

you could get a performance reduction for the UpdateAlertState usage

I didn't get the reason, please: could you better explain your concern?

? :)

EDIT: (*) I would add: that vector still remains a valid candidate as source of data. I mean:
althogth I agree with you there's no needs to parallelise Space, I think a parallelisation of some "client" class (...see the various file involved in this PR) may trigger strange behaviours.
I'm prone to change that to use 'move' explicitly, but let me know.

@jaj22

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

UpdateAlertState was already using static storage (although per-ship, so a fat source of cache misses), hence if BodyNearList wasn't returned by reference then you'd be adding allocations with the change. With return-by-reference it should be equal or better.

Any static temporary storage is going to be an obstacle to MP, but then it reduces the need for it. In a game where IIRC about half the CPU time is used for drawing the UI (and most of the rest for planets), parallelising object updates seems vanishingly far down the performance priorities.

On the other hand, one allocation per GetBodiesMaybeNear call isn't likely to make an impression on the profiler either, so feel free to remove the static storage if you'd prefer. I'm ambivalent on the point as long as there's a reserve() in there.

@mike-f1

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

UpdateAlertState was already using static storage (although per-ship, so a fat source of cache misses), hence if BodyNearList wasn't returned by reference then you'd be adding allocations with the change. With return-by-reference it should be equal or better.

I removed that caching as it seems unused.

[snip] half the CPU time is used for drawing the UI (and most of the rest for planets) [snip]

Thank's for the hint, and thank's for your answers and patience.

@fluffyfreak

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

So, is this PR ready for review and merge?

@mike-f1

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Yes

@fluffyfreak

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Ok cool, i'll review in shortly, thanks for doing it!

@mike-f1

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Ok cool, i'll review in shortly, thanks for doing it!

Np, I hope only I used the right syntax (...as you shown in your #4549,
a lot things may happens under the hood, even under the 'thin' hood
C++ provide...).

Moderator edit: It is allowed to reference issues with hyper links.

@fluffyfreak fluffyfreak merged commit 7b59822 into pioneerspacesim:master Mar 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mike-f1 mike-f1 deleted the mike-f1:bodies_near_11_and_more branch Mar 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.