Skip to content

Conversation

cypof
Copy link
Member

@cypof cypof commented Apr 25, 2015

Part of #2351, I could not switch to #2167 because I needed boost::thread forward declared. Also instead of having a max size, I prefer to use queues in pairs: free and full, so that items can be reused when the consumer is done with them. It is particularly useful for GPU memory which is costly to allocate and destroy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK will change

@jeffdonahue
Copy link
Contributor

I glanced over this and it LGTM other than my capitalization nitpick, but someone who's used boost::mutex (not me) should probably take a look. It would also be nice to have a few simple unit tests for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the first #include.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint wants the system imports first

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, my bad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think lint is wrong here with respect to the Google style guide; ideally we'll fix that at some point, but we've been following lint on this point in other places so best just to stay consistent here.

@flx42
Copy link
Contributor

flx42 commented Apr 28, 2015

LGTM once you add a proper commit message :)

@cypof
Copy link
Member Author

cypof commented Apr 28, 2015

Good point, fixed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the push method need to make an explicit unlock() call? Have you performed any timing experiments that suggest that unlocking helps in reducing lock contention?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling notify_one is best done outside the locked section. It can be done inside, but this way allows the notified thread to run immediately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some reference:
http://en.cppreference.com/w/cpp/thread/condition_variable/notify_one

The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock. However, some implementations (in particular many implementations of pthreads) recognize this situation and avoid this "hurry up and wait" scenario by transferring the waiting thread from the condition variable's queue directly to the queue of the mutex within the notify call, without waking it up.

@longjon longjon added the JL label Apr 30, 2015
@cypof cypof force-pushed the blocking_queue branch from a0a36f8 to 6108b25 Compare May 19, 2015 00:25
@shelhamer shelhamer mentioned this pull request May 30, 2015
@shelhamer
Copy link
Member

Closing since everything is bundled and updated in #2114 (and now #2870).

@shelhamer shelhamer closed this Aug 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants