Skip to content

Conversation

@dlibenzi
Copy link
Collaborator

No description provided.

@dlibenzi dlibenzi requested a review from asuhan March 17, 2019 16:29
@dlibenzi dlibenzi force-pushed the overlap_tensor_work branch 3 times, most recently from ddbe09e to 1f761a6 Compare March 17, 2019 20:37

class DeviceLocker {
public:
explicit DeviceLocker(Device device) : device_(std::move(device)) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like device_ is used, why store it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is handy to drop in debug statements on loc/unlock paths.

cv_.notify_one();
}

void Barrier() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is supposed to be per device, it's not much of a barrier. It's much more similar to a monitor / condition variable, I'd call it Wait() instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a barrier. Every async operation in flight before the barrier, must complete in order for the operations after the barrier to commence.

Equaler>;

void MaybeLRU(typename ElementList::iterator it) {
if (it != element_list_.begin()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the harm in doing this unconditionally? I wouldn't expect a measurable performance hit from doing it unconditionally, plus you can then drop the Maybe. To be fair, you could drop the Maybe anyway, since it's already LRU on the no-op path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SG. Renamed DoLRU().

Prepare XRT session so that they do not fall into the dreaded one-more-node-in-graph TF case.
Reserve XRT sessions just for allocations (sending data to device).
@dlibenzi dlibenzi force-pushed the overlap_tensor_work branch from 1f761a6 to 8fb92c8 Compare March 17, 2019 23:16
@dlibenzi dlibenzi merged commit 10b8e42 into master Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants