HenrikBengtsson / matrixStats

R package: Methods that Apply to Rows and Columns of Matrices (and to Vectors)
https://cran.r-project.org/package=matrixStats
203 stars 33 forks source link

Proof of concept optimization when both rows and cols are unspecified #216

Closed yaccos closed 3 years ago

yaccos commented 3 years ago

From the definitions of R_INDEX_OP and R_INDEX_GET, I see that these macros check both arguments for NA-values. In the cases we know that none of the operands are NA, we can avoid wasting CPU cycles on this checking. In this case, I have modified rowSums2() to skip checking for index NA values when neither row nor col subsets are provided. From my own tests, I have registered a performance boost and would like you to try it out as well. If implemented consistently, we could probably mitigate most of issue #212.

HenrikBengtsson commented 3 years ago

Good points. To clarify for others, and to make sure I got this correct, in matrixStats (<= 0.60.0), we had:

https://github.com/HenrikBengtsson/matrixStats/blob/be683978aa3c6bd8866c8e6e48aa424b47554d19/src/000.templates-types.h#L371-L377

whereas in matrixStats (>= 0.60.1), we now have:

https://github.com/HenrikBengtsson/matrixStats/blob/0158d9a34c5485cb4c7b55c164ee30a6b31bb9a2/src/000.templates-types.h#L151-L152

In other words, before the code did not check for missing values when not subsetting by rows or cols, whereas now, R_INDEX_OP and R_INDEX_GET always checks for missing values.

So, yes, I think this explains the performance degradation going from matrixStats 0.60.0 to 0.60.1.

yaccos commented 3 years ago

Sorry for my sloppy coding style, I have now taken care of that.

HenrikBengtsson commented 3 years ago

Thanks. Since this type of code has to be added to a lot of functions, I got a feeling that we might end up introducing a generic C macro for this, maybe something where one passes norows and nocols to the macro. Let's see what happens.

yaccos commented 3 years ago

I also imagine that we can check user-supplied indicies for NA-values in validateIndicies() and generalize the solution to have flags such as rowsHaveNA and colsHaveNA. In this case rows == NULL should result in rowsHaveNA == 0.