Comments (6)
I think this would benefit from a design discussion. I'm not sure I want to penalize every user of that method because someone is using the object in a non-thread-safe manner. (That method is const
from an API perspective, but actually it is a mutating method. To some extent, my knee-jerk reaction is "don't do that".)
geo_polygon
has the same issue...
from kwiver.
I think if we are having a const function, the user should not have to know that it is mutable underneath. Furthermore, we do not have direct control over how a pipeline is created. Thus, either we should just get rid of the cache map, add a mutex protection to the cache map, or we enforce for all process asking locations in different units to copy the geopoint first. I can get around this in my pipelines, but the type of pipelines written for other projects would end up running into this when they switch to the new geopoint. I mainly created this issue to document the issue.
from kwiver.
To further Juda's point, I think if the API specifies a const function users should be able to pretend it's non-mutating and be absolved from looking into a cxx file. Otherwise we're breaking the contract. It seems to me that it's the onus of the function writer to ensure that the const function behaves as const. In this case it seems that the function should be made thread-safe. It would be good to have an offline discussion with stakeholders though just to solicit more opinion.
from kwiver.
Well said @dstoup. I agree. It should be made thread safe. This probably means either putting a mutex around the map access or just ditching the map and recomputing the values instead of caching. I assume the mutex is the lesser performance hit, but I could be wrong.
from kwiver.
It would be good to have an offline discussion with stakeholders though just to solicit more opinion.
Yes, that was exactly my main point.
I can think of a couple options:
Drop the cache
Pros:
- No threading issues. Single-thread, single-conversion case is not penalized.
- Class gets smaller.
Cons:
- Repeat requests of the same conversion are penalized.
Use a mutex
We could use either a fast (even non-recursive should be safe) mutex, or an R/W mutex. The fast mutex would maximize performance in the case of zero contention, while the R/W mutex would maximize performance in the case of many requests for the same conversion.
Pros:
- Thread safe.
- Method retains "effective"
const
-ness.
Cons:
- Class gets bigger.
- Single thread users are penalized.
This is supposed to be a low-level, lightweight class. Most of the drawbacks of this approach are that it is necessarily counter to this objective.
Make the method mutable
Pros:
- Introduces no new overhead.
Cons:
- Users are responsible for synchronization where required, and must have a mutable instance in order to request conversions.
- An alternative, but less easy-to-use, API is available that would allow circumventing this requirement.
Hybrid approach
Make the caching method mutable, but also include a non-caching const
overload.
Pros:
- Does not penalize users with a mutable instance.
Cons:
- Users with a mutable instance must take care to avoid synchronization issues.
- Users with a non-mutable instance can't benefit from caching.
Which of these is the correct trade-off? I'm not sure...
from kwiver.
This has been aging for a while. Has any consensus been reached. If the race condition is still there, it seems to be rare in the wild, so the single use case looks to be the most frequent.
Is there a typical (or worst case) application where we can measure the cache hit rate? If the hit rate is low, then maybe deleting the cache is the way to go.
from kwiver.
Related Issues (20)
- Make clusters usable
- pipe-config ENV variables HOT 1
- initialize_cameras_landmarks should support per-frame initial intrinsics HOT 3
- track_set::insert should take r-value reference
- Python package automatically registers a python debug logger HOT 2
- Building tests requires sourcing `setup_KWIVER.sh` HOT 2
- Readthedocs / doxygen currently has lots of broken links and seems all broken HOT 2
- KWIVER consumers can't build against an install. Variant.hpp publicly exposed HOT 2
- File handle left open in super3d cost_volume
- Implicit cast from signed to unsigned int in ffmpeg_video_input
- Explicit cast needed in video_input_filter
- explicit cast in klv/sfm_utils HOT 1
- Potential memory leak in ffmpeg_video_input HOT 2
- Potential memory leak in SystemTools
- VisCL arrow needs to be removed
- Sprokit Embedded Pipelines With Python Processes Halts on GIL using Python 3.7+
- Vital mesh crashes on some face configurations HOT 2
- Potential seg-fault in initialize_cameras_landmarks HOT 1
- Mesh_io should throw an exception on invalid input file
- Applets do not respond to keyboard interrupt HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from kwiver.