Fix slow C3DFileAdpaterReading issue #1698 #1730

aseth1 · 2017-05-25T01:19:03Z

Fixes issue #1698
DataTable::appendRow() is slow because it is resizing the underlying SimTK::Matrix on each call, which requires reallocation and copying of the entire matrix.

Brief summary of changes

Added a protected DataTable_ constructor that takes the independent vector, data matrix and labels so that appendRow() can be avoided.

TimeSeriesTable_ exposes the convenience constructor and performs the validation and cleanup should validation fail.

C3DFileAdpater::extendRead() allocates the full time vector and data matrix first and then updates their elements and the uses the new constructor to create the TimeSeriesTable avoiding

Testing I've completed

Profiled testC3DFileAdapter prior to change the bottle neck was inside DataTable_::appendRow():
87.1 % -> _depData.resizeKeep(_depData.nrow() + 1, _depData.ncol()); (~1600ms)

After the change, the creation and updating of the matrix and construction of the full TimeSeriesTable in C3DFileAdpater::extendRead() now only uses 3.1 % of the total runtime (~34ms).

And testC3DFileAdapter passes.

Looking for feedback on...

CHANGELOG.md (choose one)

no need to update because change required an update to implementation details.

The Doxygen for this PR can be viewed at http://myosin.sourceforge.net/?C=N;O=D; click the folder whose name is this PR's number.

…e data and labels supplied. Derived classes can use this to avoid appendRow(), which resized the internal matrix.

…rom already populated time vector and data matrix.

…Table constructor and populating a pre-allocated SimTK::Matrix without resizing.

chrisdembia

Nice speedup. See my comments; all are minor.

chrisdembia · 2017-05-25T01:29:18Z

OpenSim/Common/TimeSeriesTable.h

@@ -141,6 +141,29 @@ class TimeSeriesTable_ : public DataTable_<double, ETY> {
    TimeSeriesTable_& operator=(const TimeSeriesTable_&) = default;
    TimeSeriesTable_& operator=(TimeSeriesTable_&&)      = default;
    ~TimeSeriesTable_()                                  = default;
+
+    /** Convenience Constructor to efficiently populate a time series table


Constructor -> constructor.

chrisdembia · 2017-05-25T01:31:55Z

OpenSim/Common/TimeSeriesTable.h

+            // wipe out the data loaded if any
+            _indData.clear();
+            _depData.clear();
+            removeDependentsMetaDataForKey("labels");


Would it make sense to rethrow?

chrisdembia · 2017-05-25T01:33:24Z

OpenSim/Common/TimeSeriesTable.h

+            DataTable_<double, ETY>(indVec, depData, labels) {
+        try {
+            // Perform the validation of the data a TimeSeriesTable
+            this->validateDependentsMetaData();


I see you added a comment in the base class about validation. Could you add a little more detail, either in the doxygen description for this constructor or for the one in the base class, explaining the situation with calling virtual functions from constructors?

chrisdembia · 2017-05-25T01:36:30Z

OpenSim/Common/DataTable.h

+        const SimTK::Matrix_<ETY>& depData,
+        const std::vector<std::string>& labels) {
+        OPENSIM_THROW_IF(indVec.size() != depData.nrow(), InvalidArgument,
+            "Length of independent column does not match number of rows of dependent data.");


80 columns?

chrisdembia · 2017-05-25T01:38:42Z

OpenSim/Common/C3DFileAdapter.cpp

-            marker_table.appendRow(0 + f * time_step, row);
+
+            marker_matrix.updRow(f) = row;
+            marker_times[f] = 0 + f * time_step; // pt->GetValues();


Could you either provide more detail about the comment or remove it?

chrisdembia · 2017-05-25T01:38:53Z

OpenSim/Common/C3DFileAdapter.cpp

+
+        // Create the data
+        auto& marker_table = *new 
+            TimeSeriesTableVec3 (marker_times, marker_matrix, marker_labels);


Space between Vec3 and (

chrisdembia · 2017-05-25T01:42:27Z

OpenSim/Common/C3DFileAdapter.cpp

-        for(int f = 0;
-            f < fp_force_pts->GetFrontItem()->GetFrameNumber();
-            ++f) {
+        for(int f = 0; f < force_times.size();  ++f) {


Consider using nf instead of force_times.size().

A straight replacement would cause a compiler warning (type mismatch). Would need to do

for (int f = 0; f < static_cast<int>(nf); ++f)

chrisdembia · 2017-05-25T01:43:11Z

OpenSim/Common/C3DFileAdapter.cpp

+            et->GetFrame(),
+            et->GetDescription() });
+    }
+


Is there a particular reason for moving this?

I thought it read better to get the events first, which are then added to both the marker and force tables that are created in the two code blocks below. I also thought I might be able to extract the start time for the acquisition at some point.

tkuchida

LGTM upon addressing @chrisdembia's comments |-o-|

tkuchida · 2017-05-25T04:53:19Z

OpenSim/Common/C3DFileAdapter.cpp

-        for(int f = 0;
-            f < fp_force_pts->GetFrontItem()->GetFrameNumber();
-            ++f) {
+        for(int f = 0; f < force_times.size();  ++f) {


A straight replacement would cause a compiler warning (type mismatch). Would need to do

for (int f = 0; f < static_cast<int>(nf); ++f)

…ience constructor must perform their own validation because virtual validateRow() will not invoke the derived class override during construction. Also do not exceed 80chars.

chrisdembia

I created a new issue about the initial time of the file (#1732).

aseth1 · 2017-05-25T23:24:05Z

Thank you @chrisdembia and @tkuchida. Merging.

aseth1 · 2017-05-25T23:27:28Z

@nickbianco and @jimmyDunne can you check that you are getting reasonable performance now on your big C3D files? Thanks!

nickbianco · 2017-05-31T00:24:26Z

I re-ran my test case and I'm getting reasonable performance now.

aseth1 · 2017-05-31T00:29:32Z

I re-ran my test case and I'm getting reasonable performance now.

Curious how it compares to using BTK directly, can you post the comparison as you did before in MATLAB?

nickbianco · 2017-05-31T02:55:55Z

Pretty much comparable with the update:

btk in MATLAB: 0.287 +/- 0.0329 seconds
C3DFileAdapter: 0.315 +/- 0.0067 seconds

Ajay Seth added 5 commits May 24, 2017 17:49

Added timing to C3DFileAdapter testing

46385eb

Add a protected convenience constructor to create a DataTable with th…

9b827ad

…e data and labels supplied. Derived classes can use this to avoid appendRow(), which resized the internal matrix.

Provide a convenience constructor for TimeSeriesTable_ to be filled f…

3acb82c

…rom already populated time vector and data matrix.

Speedup C3DFileAdapter::extendRead() by exploiting the new TimeSeries…

a1ee753

…Table constructor and populating a pre-allocated SimTK::Matrix without resizing.

To access member of template class need to disambiguate with this->

eb4136d

aseth1 added bug Data Performance Quick Review labels May 25, 2017

chrisdembia reviewed May 25, 2017

View reviewed changes

tkuchida approved these changes May 25, 2017

View reviewed changes

Ajay Seth added 5 commits May 24, 2017 22:54

Fix typo and clarify comment.

b99762f

Add more details about why derive class implementations of the conven…

eb0ed3c

…ience constructor must perform their own validation because virtual validateRow() will not invoke the derived class override during construction. Also do not exceed 80chars.

Address size_t->int warning and clarify comments.

eff1881

Make nf an int and use marker_nrow for number of marker frames.

3e5b56a

Address size_t -> int loss warning.

26b8135

chrisdembia approved these changes May 25, 2017

View reviewed changes

aseth1 merged commit 0470655 into master May 25, 2017

aseth1 deleted the fix_slow_C3DFileAdpater_reading_1698 branch May 25, 2017 23:25

aseth1 mentioned this pull request Dec 5, 2018

FileAdapter() performance issues with large files #2394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix slow C3DFileAdpaterReading issue #1698 #1730

Fix slow C3DFileAdpaterReading issue #1698 #1730

aseth1 commented May 25, 2017

chrisdembia left a comment

chrisdembia May 25, 2017

chrisdembia May 25, 2017

chrisdembia May 25, 2017

chrisdembia May 25, 2017

chrisdembia May 25, 2017

chrisdembia May 25, 2017

chrisdembia May 25, 2017

tkuchida May 25, 2017

chrisdembia May 25, 2017

aseth1 May 25, 2017

tkuchida left a comment •

edited

Loading

tkuchida May 25, 2017

chrisdembia left a comment

aseth1 commented May 25, 2017

aseth1 commented May 25, 2017

nickbianco commented May 31, 2017

aseth1 commented May 31, 2017

nickbianco commented May 31, 2017

Fix slow C3DFileAdpaterReading issue #1698 #1730

Fix slow C3DFileAdpaterReading issue #1698 #1730

Conversation

aseth1 commented May 25, 2017

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

chrisdembia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkuchida left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdembia left a comment

Choose a reason for hiding this comment

aseth1 commented May 25, 2017

aseth1 commented May 25, 2017

nickbianco commented May 31, 2017

aseth1 commented May 31, 2017

nickbianco commented May 31, 2017

tkuchida left a comment •

edited

Loading