Fix race condition in Python viewers set_X methods #2613
Conversation
|
I think if you rebase the CI failures will disappear since ubuntu 20.04 has since been removed |
aftersomemath
force-pushed
the
simulate-set-race
branch
from
49fd7e8 to
c9acc0a
Compare
|
Done, let's see what happens. |
simulate/simulate.cc
Outdated
| @@ -2632,18 +2632,42 @@ void Simulate::Render() { | |||
| } | |||
|
|
|||
| // user figures | |||
| if (this->newfigurerequest.load() == 1) { | |||
| this->user_figures_.clear(); | |||
| for (const auto& [viewport, figure] : this->user_figures_new_) { | |||
There was a problem hiding this comment.
Can this loop be replaced with user_figures_ = user_figures_new_?
Or better:
user_figures_.clear();
std::swap(user_figures_, user_figures_new_);
Which performs no copies (just swaps the underlying pointers).
Same for the other loops.
| for (auto& [font, gridpos, text1, text2] : this->user_texts_) { | ||
| ShowOverlayText(this, rect, font, gridpos, text1, text2); | ||
| } | ||
|
|
||
| // user images | ||
| if (this->newimagerequest.load() == 1) { |
There was a problem hiding this comment.
Make sure that if you use shared_ptr, user_images_new_ gets cleared so that the images aren't held in memory unnecessarily. I think if you use std::swap you could switch to unique_ptr.
There was a problem hiding this comment.
It was cleared on the python bindings side, but using std::swap means user_images_new_ does not need to be cleared anymore.
|
@aftersomemath, can you remove trailing whitespaces in simulate.cc ? |
|
Done, sorry about that. |
-- c9acc0a by Levi Burner <[email protected]>: Fix race condition in Python viewers set_X methods -- b89ab8f by Levi Burner <[email protected]>: use std::swap to replace some copying -- 02625be by Levi Burner <[email protected]>: fix whitespace COPYBARA_INTEGRATE_REVIEW=#2613 from aftersomemath:simulate-set-race 02625be PiperOrigin-RevId: 766646168 Change-Id: I1e1dc16afbfb1e69fb958d54bd1075cc9aed3569
|
Merged in 3eb31f5 |
The
set_figures,set_texts, andset_imagesmethods introduced by 37d7591 and #2503 have a race condition as was discussed in the review of #2503.This PR fixes that race condition. It was particularly easy to trigger when setting/clearing images, since those take more time than figures and text.
The fix uses atomics to synchronize the staging of overlay data in an almost non-blocking way. The
set_imagesmethod also now uses smart pointers to manage the image buffers.