-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Speedups to CPU shortest distance / shortest path #24
Conversation
} | ||
} | ||
} | ||
g.addGrad(std::move(arcGrads)); | ||
} | ||
|
||
std::vector<int> topSort(const Graph& g) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to throw an error if there is a loop in the graph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This top sort is much more efficient for graphs that have a lot of edges but it comes at the cost of not being able to do cycle detection. The idea is you don't have to explore every edge but you have to assume the graph is acyclic to make this assumption.
@@ -125,6 +143,21 @@ class HDSpan { | |||
return *this; | |||
}; | |||
|
|||
HDSpan& operator=(HDSpan&& other) { | |||
if (this->data_ == other.data_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think it might be better to have "==" operator defined for HDSpan where we check of data_
, device_
, and size_
are the same. We can then do
if (this == other)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The == operator is currently defined to check for semantic equality (like the elements in the two spans are the same). This is similar to std::vector.
} | ||
capacity_ = size; | ||
auto newData = allocAndCopy(data_); | ||
free(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we set size/capacity to zero in the call to free()
. Then L191 would be of no effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a little complicated at the moment :. The free is really just a convenience function for freeing the memory which first checks the device. The clear function actually sets the size to zero and frees the pointer. I could change "free" to "delete" or something like that if it would be less confusing?
TImings on my laptop using
./benchmarks/benchmark_functions
After
Timing forwardScoreLinearForward (cpu) ... 9.1351 msec
Timing forwardScoreLinearBackward (cpu) ... 9.085 msec
Timing forwardScoreRandDAGForward (cpu) ... 5.7167 msec
Timing forwardScoreRandDAGBackward (cpu) ... 6.4635 msec
Timing viterbiPathLinearForward (cpu) ... 1.7872 msec
Timing viterbiPathLinearBackward (cpu) ... 1.7198 msec
Timing viterbiPathRandDAGForward (cpu) ... 2.0626 msec
Timing viterbiPathRandDAGBackward (cpu) ... 0.1356 msec
Before
Timing forwardScoreLinearForward (cpu) ... 14.57 msec
Timing forwardScoreLinearBackward (cpu) ... 16.383 msec
Timing forwardScoreRandDAGForward (cpu) ... 5.8892 msec
Timing forwardScoreRandDAGBackward (cpu) ... 11.59 msec
Timing viterbiPathLinearForward (cpu) ... 4.2249 msec
Timing viterbiPathLinearBackward (cpu) ... 1.7191 msec
Timing viterbiPathRandDAGForward (cpu) ... 1.7667 msec
Timing viterbiPathRandDAGBackward (cpu) ... 0.087886 msec