Improve caching system

[nimo23]

Maybe we should merge these abstract classes. I dont know exactly, when to use "RecursiveCachedIndicator" and when to use "CachedIndicator".

As described in recursiveCachedIndicator:

This class is only here to avoid (OK, to postpone ???!) the StackOverflowError that may be thrown on the first getValue(int) call of a recursive indicator.

Looks for me like a workaround because of the caching layer..hence we should improve "CachedIndicator" to make the need for this "workaround" obsolete.

As the caching layer should also be improved, we could remove the "RecursiveCachedIndicator ". Maybe we can merge it all to one central point, so all indicators need to extend only one, call it "AbstractIndicator" or "BaseIndicator".

[nimo23]

How can I prevent the StackOverflow-Error:

when I use "RecursiveCachedIndicator" and call for example:

"ref.getValue(index - 1) * getValue(1) * getValue(2) * Decimal.valueOf(Math.PI * 632)"

and do some other heavy math calculations, I will get a StackOverflow-Error each time and the calculation is very slow. So how can I implement a formula at least without this StackOverflow-Error. Is this because of BigDecimal under the hood?

[nimo23]

We shold also consider to enable a caching layer which can be disabled (disabled should be the default).

[abzif]

Merging these three classes into one has some advantages and disadvantages.

Pro
Simplicity and reliability. Every indicator inherits from common superclass. Eliminating risk of stack overflow

Contra
Every indicator is computed from 0 to given index (even if we want 1000th value). So we have some time penalty, shouldn't be noticeable however.

[abzif]

Some thoughts about caching:
Tick values in general should not change (because they are from the past). Only the last (current) tick value may change. So we can cache "past" indicator values - maybe not caching the last value would be enough.

[team172011]

@nimo23

Maybe we should merge these abstract classes.

I have to take a deeper look into its logic. But I think it is not so easy to merge these

@abzif

Every indicator is computed from 0 to given index (even if we want 1000th value). So we have some time penalty, shouldn't be noticeable however.

Why is every index calculated? At the moment nothing will be calculated if you initialize an indicator. Just for the index you call in the calculate function it will be computed (a Strategy runs over all indices, it is possible that the same index will be called many times).
In my opinion the current caching logic is simple but adequate: every value that has been calculated will be cached, so you have to calculate every value at most one time per run.
Do you want another caching concept? What do you want to change or imporve?

[team172011]

I have renamed this issue. Further informations about caching plans:
See: mdeverdelhan/ta4j-origins#20, mdeverdelhan/ta4j-origins#25, mdeverdelhan/ta4j-origins#111, mdeverdelhan/ta4j-origins#138, mdeverdelhan/ta4j-origins#9 (comment)

[abzif]

@team172011
I'll try to elaborate it more:
If we construct EMA and ask for value(1000) then RecursiveCachedIndicator causes computation from index 0 to 1000 (to avoid stack overflow)
If we construct SMA and ask for value(1000) then indicator "goes back" only N items to compute value

So if all these superclasses are merged and every indicator is derived from this common "recursive cached" superclass then indicator values would be computed from 0 (even if we want only value(1000))
That's why we would have some "time penalty" for SMA - comparing to current situation.

I also think that current caching mechanism is quite simple and good - no hurry to change it. As I write earlier advantage of merging 3 superclasses would be simplicity and eliminating the risk of stack overflow.

As a small improvement I proposed not to cache the last value. That's way it would be possible to replace the last value in underlying TimeSeries and recompute last value in indicator. It's also just an idea, not something very urgent.

[team172011]

@abzif now i understand, you are right.

I also agree in not caching the last value. It is a crucial requirement that should be implemented as soon as possible

[nimo23]

The whole caching layer is actually only needed because of low calculation performance of big decimal, or?

[team172011]

I also think about to change the "setMaximumBar(int i)" concept of the time series. In the prototype implementation once can add an initial capacity to the time series during the initialization. If the bars size reach this capacity the lists will do a left shift (remove the oldest bar from beginning and insert the new bar to the end). If a shift happens, the indicators have to be notified and can also do this shift to update their cache.

[abzif]

I agree that caching layer may be bottleneck for simple calculations. But it is necessary for "self-dependent" indicators (EMA, KAMA, ZLEMA etc) to avoid stack overflow. You also don't want to endlessly recompute all prior values if you want value(1000). So it cannot be completely eliminated.

[mdeverdelhan]

If a shift happens, the indicators have to be notified and can also do this shift to update their cache.

@team172011 Actually it is already working like this (although without notification). See: https://github.com/ta4j/ta4j/blob/master/ta4j-core/src/main/java/org/ta4j/core/indicators/CachedIndicator.java#L133

By the way, for ema.getValue(1000).minus(closePrice.getValue(1000)) using BigDecimal, the current caching layer is everything but a bottleneck.

[team172011]

I think the caching system is fine and do not know what to enhance, except

• keeping the last Bar uncached Leave last bar uncached #160
• possibility to disable caching system or..
• ... posibility to define the size of the buffer like maxBarCount does, could be helpful if the user knows that his strategy mostly just needs the last 20 ticks