Minor refactor: prevent string copying, list -> vector, shared_ptr by… #8872
Conversation
| @@ -122,7 +123,7 @@ class ThreadPool { | |||
| /*! | |||
| * \brief Startup synchronization objects | |||
| */ | |||
| std::list<std::shared_ptr<SimpleEvent>> ready_events_; | |||
| std::vector<std::shared_ptr<SimpleEvent> > ready_events_; | |||
There was a problem hiding this comment.
Are you sure about this? I think this is by design
There was a problem hiding this comment.
I checked the code several times, why use a list if we are just appending items, which are in addition pointers. I don't think it's a critical code path, correct me if I'm wrong, but in general a vector is faster,smaller and cache efficient. Do you see any problems with this?
There was a problem hiding this comment.
Checked again, should be fine.
There was a problem hiding this comment.
Why did you change to vector?
There was a problem hiding this comment.
Inserting arbitrarily into a vector can cause CPU spikes. These are not accessed in a way that cache matters and random access is not required. Please revert.
|
please change type const -> const type |
larroy
force-pushed
the
refactor
branch
8 times, most recently
from
b28050a to
e139608
Compare
larroy
force-pushed
the
refactor
branch
4 times, most recently
from
fb0e257 to
9360b04
Compare
|
LGTM. |
larroy
force-pushed
the
refactor
branch
2 times, most recently
from
3b19270 to
dd62dbd
Compare
|
Done |
|
@piiswrong please merge? |
|
@cjolivier01 this should be good to go as well. Thank you so much for merging my PRs. |
| @@ -58,8 +57,8 @@ class ThreadPool { | |||
|
|
|||
| /*! \brief Signal event upon destruction, even for exceptions (RAII) */ | |||
| struct SetReadyOnDestroy { | |||
| explicit inline SetReadyOnDestroy(std::shared_ptr<SimpleEvent> *event) | |||
| : event_(*event) { | |||
| explicit inline SetReadyOnDestroy(const std::shared_ptr<SimpleEvent>& event) | |||
There was a problem hiding this comment.
general practice is to pass shared pointers by reference to prevent copies and in this case the pointer to shared pointer could be null and is not being checked.
|
Changing the strings to references instead of unnecesary copies is not just for the sake of changing them. |
|
changed the vector back to list |
|
Actually I disagree with your statement about the vector. The size is fixed and known at construction time. There's not going to be any cpu spike. Please elaborate your claim. |
Checklist
Essentials
make lint)Changes