JetBrains-Research / viktor

Efficient f64-only ndarray in Kotlin
MIT License
125 stars 6 forks source link

Unnecessarily complex native functions #17

Closed dievsky closed 5 years ago

dievsky commented 5 years ago

Let's take, for example,

external fun unsafePlus(
        src1: DoubleArray, srcOffset1: Int,
        src2: DoubleArray, srcOffset2: Int,
        dst: DoubleArray, dstOffset: Int, length: Int
)

It's internal, so not visible from the outside, and a quick "Find Usages" shows that it's only used once, in a context:

NativeSpeedups.unsafePlus(
    data, offset,
    other.data, other.offset,
    data, offset, size
)

In other words, dst and dstOffset from the original signature are always equal to src1 and srcOffset1.

Maybe we should change the C++ function's signature and logic to reflect this. It might allow the compiler to produce more efficient bytecode. Or maybe we should expose this logic to the library user.

dievsky commented 5 years ago

The only API function that exposes this logic is

open class F64Array {
// ...
    fun logAddExp(other: F64Array, dst: F64Array)
// ...
}

But even in this case every actual usage of the function in our projects has dst == this.

dievsky commented 5 years ago

After some deliberation, here's my opinion: exposing a bulky API that will be used by about one and a half developer and will just allow to skip one array allocation/copying per operation is probably not really worth it. Allocation+copy is fairly cheap (citation needed). Thus I vote for simplifying the native C++ signatures to mimic plusAssign and expInPlace logic (i.e. replace dst with src1 or src respectively). I also propose to deprecate the current F64Array.logAddExp(other, dst) Kotlin method and offer a F64Array.logAddExpAssign(other) (or something less atrociously named) instead.

dievsky commented 5 years ago

Fixed by 1efb244c0f11dccf93ff3588709735485151a957.