Use Index object for NativeTypes instead of index fields

[tpietzsch]

This addresses a polymorphism issue discovered by @maarzt:
There are multiple implementations of NativeType.incIndex() etc, and when using e.g. a RandomAccess to move over images of different types, this slows things down. Also in the case that the type is always the same at the call site.

The problem is illustrated by this benchmark where pixels of a 1000x1000 image are summed using a ArrayRandomAccess. If different types than the one of the image have been observed anywhere else in the program this operation slows down by 4x. This can be traced to NativeType.updateIndex() etc. This is obviously a problem for a lot of things using imglib.

    Benchmark                       (slowdown)  Mode  Cnt  Score   Error  Units
    ArrayRandomAccessBenchmark.sum       false  avgt   10  0.982 ± 0.011  ms/op
    ArrayRandomAccessBenchmark.sum        true  avgt   10  3.942 ± 0.102  ms/op

This PR introduces an Index class that is used by all NativeType implementations to store their index.
Accessors can directly obtain (and cache) this object and use it to modify the types index.
This solves the issue:

    Benchmark                       (slowdown)  Mode  Cnt  Score   Error  Units
    ArrayRandomAccessBenchmark.sum       false  avgt   10  1.052 ± 0.019  ms/op
    ArrayRandomAccessBenchmark.sum        true  avgt   10  1.046 ± 0.019  ms/op

(As far as I can tell, using the Index object instead of a primitive int field does not cause performance overhead.)

The only issue with this is that the types deriving from AbstractBitType had a long index before and now, through Index, implicitly have int indexes. This means that while you could create an AbstractBitType that would run on primitive arrays of more than Integer.MAX_VALUE bits before, you no longer can. I don't think this is an actual issue. As far as I can tell, it never really would work in any concrete use case because the int index assumption is baked in deeply in all Accessors etc.
I suggest that we just live with it for now.

If there are actual use cases that bypass images and accessors and do something directly with those types, I would suggest that we simply create a parallel class hierarchy explicitly for those scenarios.

[maarzt]

You found an ingeniously simple solution to this problem. I like it very much. 🥳

Overall performance improvement in Labkit is 20 %. That's quite some margin.

[axtimwalde]

Awesome! Thanks!

[maarzt]

Looks good to me ;)

[maarzt]

I find it slightly annoying that this field is named "i" in CellCursor while it is named "index" in AbstractArrayCursor.

I understand the reason that the name is clashing with the already existing protect int index. Maybe the exiting field could be removed now? Or maybe "typeIndex" could be used as a name everywhere consistently?

[tpietzsch]

Yes, I also didn't like that. I didn't want to make any changes unrelated to the problem for now.

We could swap the names 'i' and 'index' in the CellCursor.
Maybe the existing field could be removed, we would need to do benchmarks though. I remember that we spent a lot of time tweaking performance but that was before JMH. Maybe the current polymorphism issue played into the measurements and also maybe the JIT changed to favour different patterns.

In any case we will have to check that there is no derived class somewhere that uses the existing 'index'.

It would be good to revisit, but in a separate issue.
(Also #304 that I stumbled across while making the changes...)

[maarzt]

I found this this class in KNIME image processing that overloads CellCursor and uses the index field.

[maarzt]

Let's either merge this branch as it is, or rename all the fields to something consistent, like typeIndex.

[axtimwalde]

I find typeIndex a good compromise as it is unambiguous and is unlikely to create surprises (such as in for (int i ... loops).

[hanslovsky]

Suggested change

	public void set( final int index)
	public void set( final int index )

[hanslovsky]

This looks good to me with the caveat that I only looked at it at a high level without diving into every single file. This should automatically improve performance for practically every single downstream code base. Trying summarize in my own words why this fix works (please confirm that this is correct; may be helpful for others):
With NativeType.incIndex(), we pay the polymorphism penalty at every single pixel. With the new Index class, we pay the penalty only at creation time of the RandomAccess or Cursor. When moving RandomAccess or Cursor, we call Index.inc() (or any other method on Index) the JVM always sees the same class, so there is no polymorphism penalty when moving from pixel to pixel. Making Index final prevents accidental introduction of polymorphism in downstream code.

Great fix. I am in favor of the name change to typeIndex as suggested by @maarzt

[tpietzsch]

I did the typeIndex renaming and merged it. Thank you everybody for reviewing!