Speed up adding fixings

[pcaspers]

We rely on Index::addFixing() to add historical fixings. When we add a large number of fixings (e.g. 10k - 100k) we see that this takes quite long, in the order of seconds, maybe 10s.

There is Index::addFixings() which is called from Index::addFIxing() and allows to add multiple fixings at once, but it can not be used generically since inflation indices overrides Index::addFixing() with their own logic. We have some custom index implementations which overwrite this method too.

In the end, they all call into Index::addFixings() though, so it seems this is the right place for a performance optimization. This is what I am trying to do here. I observe a speed up of >100x when many fixings are set, this is achieved by avoiding the copy of the time series storing the historical fixings on each call of Index::addFixings().

[lballabio]

You're right, we should review this, but this way index observers don't get a notification when the fixing is added (which currently happens when the ObservableValue is reassigned).

We should probably skip wrapping the history in the ObservableValue and write a custom implementation of addFixing(s) that notifies observers explicitly.

[lballabio]

On the other hand, good to see that the tests caught the problem :)

[pcaspers]

Yes they did. I just proceeded with "minimal design thoughts", which does the job. We can of course discuss how to simplify that further.

[pcaspers]

Ok now there is some obvious overlap between my new getHistoryObservableValueRef() and notifier()...

[pcaspers]

So based on the changes here, would you remove the ObservableValue<> wrapper around TimeSeries<Real> and then provide a new IndexManager::notifier(name) implementation?

[lballabio]

I'm not sure. A method name like getHistoryObservableValueRef obviously says that we're reaching into the guts of the class to get something that we shouldn't even be aware of :)

Off the top of my head I would give IndexManager an addFixing(name, date, fixing) method (and probably addFixings too) so that it can do the work itself; Index::addFixing would forward to it. This way we can avoid the getHistoryObservableValueRef dance so the guts of the class stay inside the class. And yes, we can avoid ObservableValue, and notifier should have another implementation. Maybe the manager can have a map of timeseries and a map of notifiers, or something like that. As I said, off the top of my head so take it with a grain of salt.

[pcaspers]

Ok I can try to clean that up, thanks.

[pcaspers]

The Codacy error looks like a false positive. Can I fix that myself by adding //NOLINT(expr) and how can I determine what exprshould be?

Not sure about the checks that are still yellow?

[pcaspers]

Ah I changed the behavior of Index::addFixings() when an invalid fixing is provided, let me change that back

[pcaspers]

I happy with this now, seeing the desired performance improvement and all tests on our side passing.

[lballabio]

The checks were yellow because the commits were pushed by @damienbarker and he didn't contribute before (as in, he didn't have a commit authored by him merged) so GitHub waits for my approval before running the tests. It's something they added a while ago because people were opening fake PRs all around GitHub to mine Bitcoin in the CI builds...

[lballabio]

Also, I was thinking about maybe hiding the new methods of IndexManager and making Index a friend, so we'd have a single public interface (through Index) and not two. Possibly some of the old methods, too? What do you think?

[pcaspers]

Ah yes, that's because I pushed the commits via our own CI which uses that user.

And yes I can hide the new method, will also check the old ones.

[lballabio]

Ok, thanks. No hurry, though, I'm cutting 1.36 in the next days so I'll get back to this later—I'd be a bit uncomfortable putting this in at the last minute.

[pcaspers]

Sure!

[coveralls]

coverage: 72.673% (-0.004%) from 72.677%
when pulling 0e52676 on OpenSourceRisk:addfixings_speedup
into 8bd5703 on lballabio:master.

[pcaspers]

I hid the newly added methods. I think we should deprecate other public methods in IndexManager before hiding them though? Let's discuss after the 1.36 release.