Use std::shared_ptr in case if ENABLE_THREAD_SAFE_TILE_REF_COUNT is ON

[SiarheiFedartsou]

Issue

During implementation of #2730 we learnt that using of std::shared_ptr(which has such ability) has certain performance impact, that's why we ended up using boost::intrusive_ptr. Then we added ENABLE_THREAD_SAFE_TILE_REF_COUNT to work around issues with non thread-safe reference counter in multithreading environment. Since ENABLE_THREAD_SAFE_TILE_REF_COUNT is supposed to be used in the cases when performance is not top-1 priority and sometimes it is very useful to be able to get weak pointer to tile I propose to use std::shared_ptr as tile pointer in case of ENABLE_THREAD_SAFE_TILE_REF_COUNT(instead of boost::intrusive_ptr with thread-safe counter).

Tasklist

• Add tests
• Add #fixes with the issue number that this PR addresses
• Update the docs with any new request parameters or changes to behavior described
• Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

[averkhaturau]

lgtm

[mandeepsandhu]

Is getting a weak pointer from a shared ptr the main motivation for this change?

[kevinkreiser]

Looks good to me. I wish we could have just used shared_ptr regardless but the perf penalty was too much.

[SiarheiFedartsou]

Is getting a weak pointer from a shared ptr the main motivation for this change?

Yes. Do you have alternative ideas how it can be solved?

[mandeepsandhu]

Is getting a weak pointer from a shared ptr the main motivation for this change?

Yes. Do you have alternative ideas how it can be solved?

No, I think your solution is good! I was wondering if we still need to have intrusive_ptr for the non-thread safe usecase?

Minor optional nit: we could rename ENABLE_THREAD_SAFE_TILE_REF_COUNT to something like ENABLE_THREAD_SAFE_TILE_PTR (since we're not explicitly managing a ref counter). I'm fine leaving it as-is too.

[kevinkreiser]

@mandeepsandhu the only reason we have it at all is performance issues. iirc it was about a 6% performance reduction

[mandeepsandhu]

@mandeepsandhu the only reason we have it at all is performance issues. iirc it was about a 6% performance reduction

6% reduction compared to using shared_ptr in single or multi-threaded context?

[kevinkreiser]

24 thread load test using mem mapped tar with no locking needed (every thread has its own graph reader)

[danpat]

@kevinkreiser it's usually strongly encouraged to use const references to avoid triggering shared_ptr internal mutexes - is not doing that everywhere possibly the source of the slowdown, and perhaps a less kafkaesque fix might be to fix the usage pattern of graph tile pointers?

[kevinkreiser]

@danpat good question! at the end of the day the various TileCache implementations hold the original data. it is possible that you clear the tile cache while still holding a reference to a graph tile. owning your own shared_ptr removes the possibility that the one you have gets deallocated out from underneath you but this was only a feature we gained recently (when @kkaefer and @averkhaturau did the tilecache and shared_ptr refactors). i guess the real question is who owns the lifetime of the tile, i could see a case for the caller to want to take ownership and that happens by default when returning non const reference but we could force use-cases that need that to opt in by copying the shared_ptr explicitly. the way the code is written today i'm pretty certain we'd be fine switch to returning a reference to the shared_ptr from the tilecache. downstream users of the library though may rely on the ability to transfer the lifetime of the tile in question out of the tile cache and into the callers scope. @mskurydin might have some opinions there too possibly

[SiarheiFedartsou]

Minor optional nit: we could rename ENABLE_THREAD_SAFE_TILE_REF_COUNT to something like ENABLE_THREAD_SAFE_TILE_PTR (since we're not explicitly managing a ref counter). I'm fine leaving it as-is too.

The proposed name is definitely better, but I don't want to break backward compatibility(some Valhalla users use this flag already) and probably I'll leave it as is.