Increase performance of collection deepMap and deepForEach with DenseMatrices

[dvd101x]

Hi, this PR increases performance of deepMap and deepForEach when iterating over a DenseMatrix in collections by using a non indexed version of ._forEach and also forcing the optimizeCallback function to return a function with one argument.

[dvd101x]

I also would like to mention that currently the function norm, uses the second argument of DenseMatrix.forEach as a skipZeros, but it doesn't work.

That's why it is an argument here, but no implementation is made.

The next step of addressing #3262 is for optimizeCallback to return arity information so that the fast non indexed iteration can be used if possible. I will address that in another PR.

[dvd101x]

In this latest commit, the non indexed version of the iteration functions is implemented when possible, with these benefits (about 25% faster as a maximum)

This includes an arity check for regular functions as stated at #3262 which I think was accurate and concerned about the big gap between abs(array) and the rest.

I think this PR could open a discussion about a state of diminishing returns due to the indexed methods being previously optimized by various PR, if we compared with V13.1.1 before all the PR improving the indexed algorithms the benefits are huge.

I'm open to any discussions this data might arise as I found #3262 intriguing, otherwise I think this PR could be implemented as such.

[dvd101x]

Also I would like to know if it's better to pass an options object to skipIndex and skipZeroes and include that in the documentation for DenseMatrix methods or if it's better like individual boolean parameters.

[josdejong]

We've made huge improvements since v13.1.1! I guess we've addressed all the low hanging fruit now, so maybe time to stop 😜. The 25% improvment in this PR is worth it I think, since map and forEach are used a lot.

I made a couple of inline comments, can you have a look at those?

[dvd101x]

Thanks, I looked into your comments and I will address them individually, in my notes I recall another bottleneck and that's it.

The bottleneck occurs when the callback optimization is run twice and sometimes is not run. I'm considering adding attributes to callbacks and arrays, the eliminate the need to pass extra arguments in some functions. I will review this and maybe next week have a response.

[josdejong]

The bottleneck occurs when the callback optimization is run twice and sometimes is not run.

Would be good to have a look at that, but please do a quick check first to see if the callback optimization is indeed a bottleneck, I would expect the callback optimization to take a fraction of the time compared to running the callback for every element on a (large) matrix, so maybe there is not much to gain there.

[dvd101x]

but please do a quick check first to see if the callback optimization is indeed a bottleneck

Thank's, my intuition was that maybe if it does a double try catch on each iteration and a double check for the number of arguments might affect in some way.... but thinking about it, in many cases it returns a regular function calling the typed function with the improved error message, and since the improved error message happens only for typed functions I think it should be ok. Nonetheless, I might do a quick check.

In the latest commit most comments are addressed and these are the benchmarks.

Just a few comments:

• I kept the use of maxDepth as it did affect performance
• I think it's best to wait for Improve performance of flatten in DenseMatrix #3400 and then use Matrix.forEach since there is no SparseMatrix._forEach.
• About removing the second argument skipZeros in DenseMatrix I just noticed that SparseMatrix uses the second argument skipZeros and it's also part of the jsdocs of Matrix.map. So I don't know if this changes anything.

[dvd101x]

As a separate topic, at some point I would like to address the duplicate logic as suggested at #3266

In #3251, @Galm007 has created a single _forEach method used by both map and forEach methods to deduplicate the logic. It may be worth trying to let deepMap use deepForEach under the hood to deduplicate logic of this PR too. What do you think?

I'm thinking of including that after this PR without affecting performance significantly.

[josdejong]

I see bigger performance improvements than in your last chart, that is a good thing :)

Merging the PR now.

[dvd101x]

Hi, I think this is ready to merge.