Insert to bimaps left instead of directly to avoid crashes in caching

[YakoYakoYokuYoku]

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:

• I've followed the contributing guidelines to the best of my understanding
• I've read and understood the contributing guidelines
• I've formatted my code according to Natron's code style
• I've searched the pull requests tracker to ensure that this PR is not a duplicate

PR Description

What type of PR is this? (Check one of the boxes below)

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (non-breaking change which does not add functionality nor fixes a bug but improves Natron in some way)
• Breaking change (fix or feature that would cause existing functionality to change)
• My change requires a change to the documentation
  • I have updated the documentation accordingly

What does this pull request do?

With Boost 1.76.0 (at least) Natron crashes when a cache run is triggered. AFAICT the LRU insertion of keys leaves the bimaps in a corrupted state and when the entries have to be iterated the crash is reproduced. Accourding to the Boost bimap docs inserting to the left map of a bimap is recommended instead of doing it directly, at least an extra call is avoided. By doing so I do not longer encounter the crash. The recomendation is valid from 1.72.0, I have yet to see if previous versions accept inserting to the left map.

Show a few screenshots (if this is a visual change)

N/A.

Have you tested your changes (if applicable)? If so, how?

Built Natron, laid out a nodegraph and hit play to preview it.

Futher details of this pull request

N/A.

[devernay]

What worries me is that the previous version is still supposed to work (not crash) according to the doc: it just adds a call to a copy constructor.

Could you find an explanation for the crash?

[YakoYakoYokuYoku]

I cannot explain what produces the crash inside Boost although what I can do is share the following backtrace

#0  0x0000000000a06866 in boost::multi_index::multi_index_container<boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true>, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na>::core_indices, std::allocator<boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true> > >::header() const (this=0xfffffffffffffff0)
    at /usr/include/boost/multi_index_container.hpp:639
#1  0x0000000000a03f12 in boost::multi_index::detail::index_base<boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true>, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na>::core_indices, std::allocator<boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true> > >::final_header() const (this=0x0)
    at /usr/include/boost/multi_index/detail/index_base.hpp:206
#2  0x0000000000a04318 in boost::multi_index::detail::hashed_index<boost::multi_index::member<boost::bimaps::relation::detail::relation_storage<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, true>, unsigned long, &boost::bimaps::relation::detail::relation_storage<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, true>::left>, boost::hash<unsigned long>, std::equal_to<unsigned long>, boost::multi_index::detail::nth_layer<2, boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true>, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na>::core_indices, std::allocator<boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true> > >, boost::mpl::v_item<boost::bimaps::relation::member_at::left, boost::mpl::vector0<mpl_::na>, 0>, boost::multi_index::detail::hashed_unique_tag>::header() const (this=0x0) at /usr/include/boost/multi_index/hashed_index.hpp:1199
#3  0x0000000000a02506 in boost::multi_index::detail::hashed_index<boost::multi_index::member<boost::bimaps::relation::detail::relation_storage<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, true>, unsigned long, &boost::bimaps::relation::detail::relation_storage<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, true>::left>, boost::hash<unsigned long>, std::equal_to<unsigned long>, boost::multi_index::detail::nth_layer<2, boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true>, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na>::core_indices, std::allocator<boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true> > >, boost::mpl::v_item<boost::bimaps::relation::member_at::left, boost::mpl::vector0<mpl_::na>, 0>, boost::multi_index::detail::hashed_unique_tag>::begin() (this=0x0) at /usr/include/boost/multi_index/hashed_index.hpp:247
#4  0x00000000009ff38e in _ZN5boost6bimaps17container_adaptor17container_adaptorINS_11multi_index6detail12hashed_indexINS3_6memberINS0_8relation6detail16relation_storageINS0_4tags6taggedIKmNS7_9member_at4leftEEENSB_INSt7__cxx114listINS_10shared_ptrIN6Natron10FrameEntryEEESaISL_EEENSD_5rightEEELb1EEEmXadL_ZNSQ_4leftEEEEENS_4hashImEESt8equal_toImENS4_9nth_layerILi2ENS7_15mutant_relationISF_SP_N4mpl_2naELb1EEENS0_6detail10bimap_coreINS0_16unordered_set_ofImST_SV_EENS0_7list_ofISN_EESZ_SZ_SZ_E12core_indicesESaIS10_EEENS_3mpl6v_itemISE_NS1B_7vector0ISZ_EELi0EEENS4_17hashed_unique_tagEEENS11_17map_view_iteratorISE_S17_EENS11_23const_map_view_iteratorISE_S17_EENS1_7support23iterator_facade_to_baseIS1J_S1L_EESZ_NS8_24pair_to_relation_functorISE_S10_EENS7_7support16get_pair_functorISE_S10_EENS1C_INS1_6detail20key_to_base_identityImSC_EENS1C_INS1U_27iterator_from_base_identityINS4_21hashed_index_iteratorINS4_17hashed_index_nodeINS4_15index_node_baseIS10_S19_EEEENS4_12bucket_arrayIS19_EES1G_NS4_31hashed_index_local_iterator_tagEEENS11_23local_map_view_iteratorISE_S17_EES26_NS11_29const_local_map_view_iteratorISE_S17_EEEENS1B_6vectorISZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_SZ_EELi1EEELi1EEEE5beginEv (this=0x7ffec6ffc3a0) at /usr/include/boost/bimap/container_adaptor/container_adaptor.hpp:165
#5  0x00000000009fa4aa in boost::bimaps::container_adaptor::unordered_associative_container_adaptor<boost::multi_index::detail::hashed_index<boost::multi_index::member<boost::bimaps::relation::de--Type <RET> for more, q to quit, c to continue without paging--
tail::relation_storage<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, true>, unsigned long, &boost::bimaps::relation::detail::relation_storage<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, true>::left>, boost::hash<unsigned long>, std::equal_to<unsigned long>, boost::multi_index::detail::nth_layer<2, boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true>, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na>::core_indices, std::allocator<boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true> > >, boost::mpl::v_item<boost::bimaps::relation::member_at::left, boost::mpl::vector0<mpl_::na>, 0>, boost::multi_index::detail::hashed_unique_tag>, boost::bimaps::detail::map_view_iterator<boost::bimaps::relation::member_at::left, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na> >, boost::bimaps::detail::const_map_view_iterator<boost::bimaps::relation::member_at::left, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na> >, boost::bimaps::detail::local_map_view_iterator<boost::bimaps::relation::member_at::left, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na> >, boost::bimaps::detail::const_local_map_view_iterator<boost::bimaps::relation::member_at::left, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na> >, unsigned long const, boost::bimaps::container_adaptor::support::iterator_facade_to_base<boost::bimaps::detail::map_view_iterator<boost::bimaps::relation::member_at::left, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na> >, boost::bimaps::detail::const_map_view_iterator<boost::bimaps::relation::member_at::left, boost::bimaps::detail::bimap_core<boost::bimaps::unordered_set_of<unsigned long, boost::hash<unsigned long>, std::equal_to<unsigned long> >, boost::bimaps::list_of<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > > >, mpl_::na, mpl_::na, mpl_::na> > >, mpl_::na, mpl_::na, boost::bimaps::relation::detail::pair_to_relation_functor<boost::bimaps::relation::member_at::left, boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true> >, boost::bimaps::relation::support::get_pair_functor<boost::bimaps::relation::member_at::left, boost::bimaps::relation::mutant_relation<boost::bimaps::tags::tagged<unsigned long const, boost::bimaps::relation::member_at::left>, boost::bimaps::tags::tagged<std::__cxx11::list<boost::shared_ptr<Natron::FrameEntry>, std::allocator<boost::shared_ptr<Natron::FrameEntry> > >, boost::bimaps::relation::member_at::right>, mpl_::na, true> >, mpl_::na, boost::mpl::vector<mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na> >::begin() (this=0x7ffec6ffc3a0)
    at /usr/include/boost/bimap/container_adaptor/unordered_associative_container_adaptor.hpp:267
#6  0x00000000009f14c2 in BoostLRUHashTable<unsigned long, boost::shared_ptr<Natron::FrameEntry> >::begin[abi:cxx11]() (this=0x7ffec6ffc340) at ../../../Natron/Engine/LRUHashTable.h:512
#7  0x0000000000a2164e in Natron::Cache<Natron::FrameEntry>::save(std::__cxx11::list<Natron::Cache<Natron::FrameEntry>::SerializedEntry, std::allocator<Natron::Cache<Natron::FrameEntry>::SerializedEntry> >*) (this=0x2481b80, tableOfContents=0x7ffec6ffc570) at ../../../Natron/Engine/CacheSerialization.h:82
#8  0x0000000000a1f049 in Natron::saveCache<Natron::FrameEntry>(Natron::Cache<Natron::FrameEntry>*) (cache=0x2481b80) at ../../Engine/AppManagerPrivate.cpp:473
#9  0x0000000000a19b83 in Natron::AppManagerPrivate::saveCaches() (this=0x1e09bc0) at ../../Engine/AppManagerPrivate.cpp:488
#10 0x00000000009c667e in Natron::AppManager::saveCaches() const (this=0x7fffffffdf90) at ../../Engine/AppManager.cpp:258
#11 0x0000000000eb1783 in Natron::Project::saveProject_imp(QString const&, QString const&, bool, bool, QString*) (this=
    0x344db70, path=..., name=..., autoS=true, updateProjectProperties=true, newFilePath=0x0) at ../../Engine/Project.cpp:477
#12 0x0000000000eb300f in Natron::Project::autoSave() (this=0x344db70) at ../../Engine/Project.cpp:654
#13 0x0000000000ed5b34 in QtConcurrent::VoidStoredMemberFunctionPointerCall0<void, Natron::Project>::runFunctor() (this=0x5961310)
    at /usr/include/QtConcurrent/qtconcurrentstoredfunctioncall.h:205
#14 0x00000000004a3cc1 in QtConcurrent::RunFunctionTask<void>::run() (this=0x5961310) at /usr/include/QtConcurrent/qtconcurrentrunbase.h:136
#15 0x00007ffff621f042 in  () at /usr/lib/libQt5Core.so.5
#16 0x00007ffff621be21 in  () at /usr/lib/libQt5Core.so.5
#17 0x00007ffff60ad41d in start_thread (arg=0x7ffec6ffd640) at pthread_create.c:481
#18 0x00007ffff5be68d3 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

[devernay]

I still have issues with this change:

• the proposed change is in an inactive section of the code. The active section goes from line 472 to line 573 (and we should probably cleanup this code at some point). I don't understand how a change to an inactive section can fix a bug
• there is another call to _container.insert just above this one (even in the active section). Shouldn't it also be replaced with _container.left.insert?

[devernay]

I reverted this change, because:

• all bimap examples in boost 1.78 insert in the relation and not in the left map
• the statement from the documentation concerns copying from an std::map to a boost bimap. In fact, only the left view of a bimap is signature-compatible with a std::map.

In the statement, "However" refers to copying from a std::map:

This mean that you can insert elements in a bimap using algorithms like std::copy from containers like std::map, or use std::make_pair to add new elements. However it is best to use bm.left.insert( bm_type::left_value_type(f,s) ) instead of bm.insert( std::make_pair(f,s) ) to avoid an extra call to the copy constructor of each type.

Sorry, but the real bug is probably elsewhere, and it's more probably a race condition

[YakoYakoYokuYoku]

Probably it is, although I don't really know how exactly I was reproducing it many times while trying to test #733 in RB-2.5.

[devernay]

This looks 90% like a race condition, as with many cache-related bugs:

• in the crash above, you see that Natron is auto-saving the project and the caches
• my guess is that the cache is not properly protected, and while it's saving the cache, another thread is modifying it