-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make floating-rate coupons lazy #1566
Conversation
updated version of #1222 |
Some details. The main idea of the PR is to make floating-rate coupons lazy (as in, inherited from The possible problems are triggered as follows:
This happened in a couple of places in the library, and was fixed with ad-hoc solutions; in Possible solutions:
All of these solutions have pros and cons and should be timed in stress scenarios. Suggestions are welcome. |
I'd be in favour of 2 - I am happy to change the PR to do 2 and run some tests on our side to check the performance. |
…ect registers with it
I made this change for "2" in the last commit here. I am not sure if it is the best way (in terms of performance) to make |
ok so MSVC complains about this warning C4250: 'QuantLib::CapFloorTermVolSurface': inherits 'QuantLib::LazyObject::QuantLib::LazyObject::registerWith' via dominance and other similar cases, but before trying to fix this we should probably decide whether this is the way to go? |
Hi Peter, can you try to build the whole PR on VS 2022 with the options I could replicate the memory leak I have with the change to the virtual inheritance in It doesn't have any warnings but crashes on VS 2022 with
but does not compile with
Without
Regards Ralf |
Hi @ralfkonrad so in short the problem goes away when enabling the c++17 standard (except for the compiler warning you get)? |
Yeah, looks like that. Also the code doesn't cause problems on |
It could be worth sending a bug report to microsoft on that including your minimal example? I guess this warning
is also a bug. |
Yes, that was the idea behind this example. Not sure about the @sweemer: Do you have an opinion here?! The minimal example is "inspired" by a bug on our side using the virtual inheritance in |
@pcaspers Besides that I have also tested the performance and the PR doesn't look to have an impact (on our use case). 👍 |
Hi all, I'm still trying to get caught up to speed on these changes. I can't yet say I fully understand it, but I do have some initial high-level thoughts.
Regarding the compiler bug, hopefully Microsoft can quickly fix it based upon your nice repro. |
Here is the corresponding bug report: https://developercommunity.visualstudio.com/t/Visual-Studio-2022-Segmentation-Fault-wi/10263539 |
@ralfkonrad By the way, does your real code where you first encountered the memory leak also delegate the explicit constructor to the copy or move constructor? If so, can you try implementing the explicit constructor without delegation and see if that fixes the issue? Of course that doesn't help with the compiler bug but it might help unblock you or others in the meantime. |
@sweemer Yes, the real code also delegate the explicit constructor to the copy or move constructor. However as often, implementing the explicit constructor without delegation is not as straight forward as in the simplified example. We have class OurFloatingRateCoupon {
public:
OurFloatingRateCoupon (const shared_ptr<Cashflow>& cf)
: ql::FloatingRateCoupon(
makeCF(cf).a(),
makeCF(cf).lot(),
makeCF(cf).of(),
makeCF(cf).parameters(),
...,
) {}
private:
static FloatingRateCoupon makeCF(const shared_ptr<Cashflow>& cf) {
// Do the casting etc.
return FloatingRateCoupon(
a,
lot,
of,
parameters,
...
);
}
} Also ugly... The "workaround" class ChildWithNoProblem : public Parent {
public:
ChildWithNoProblem() = default;
explicit ChildWithNoProblem(const std::shared_ptr<Parent>& parent)
: ChildWithNoProblem(*makeChild(parent)) {}
private:
static std::shared_ptr<ChildWithNoProblem> makeChild(const std::shared_ptr<Parent>& parent) {
std::cout << "\t Casting ChildWithNoProblem..." << std::endl;
auto child = std::dynamic_pointer_cast<ChildWithNoProblem>(parent);
std::cout << "\t Done..." << std::endl;
return std::make_shared<ChildWithNoProblem>();
}
}; |
@lballabio this comment might have gone unnoticed: "ok so MSVC complains about this warning C4250: 'QuantLib::CapFloorTermVolSurface': inherits 'QuantLib::LazyObject::QuantLib::LazyObject::registerWith' via dominance and other similar cases, but before trying to fix this we should probably decide whether this is the way to go?" I am happy to resolve this unless you say we should further explore the approach to take? Of the four alternatives you presented initially, this one seems the most straightforward + least error-prone one. If we should run into performance issues, we can still refine the approach? |
I think that warning can be suppressed, because it's just informational and not actually highlighting anything unexpected. Speaking of possibly unnoticed comments, any thoughts about my idea to move the |
Yes, let's go ahead. I think we can avoid the warning by giving |
ok thanks, I'll try to get rid of the warnings @sweemer not sure about the best level to introduce LazyObject in the inheritance tree (well, it's not a tree, unfortunately...). So far my intuition was to do this as far down (i.e. towards the most specialised classes) as possible and only if actually backed up by a real use case to avoid introducing unnecessary overhead for classes that actually do not benefit from the lazy evaluation. Although can lazy evaluation ever be bad .. :). So maybe it makes sense what you are suggesting. |
True! I fully agree,
|
No, sorry. It was an |
One thing that might mitigate the problems with only forwarding the first notification: the tricky cases are those in which the evaluation date changes, right? And in those cases, we should probably recalculate anyway. What if the constructor of |
Two big differences I see are that the former Rate FloatingRateCoupon::rate() const {
QL_REQUIRE(pricer_, "pricer not set");
pricer_->initialize(*this);
return pricer_->swapletRate();
}
void FloatingRateCoupon::update() override { notifyObservers(); } now effectively became Rate FloatingRateCoupon::rate() const {
if (!calculated_ && !frozen_) {
calculated_ = true; // prevent infinite recursion in
// case of bootstrapping
try {
QL_REQUIRE(pricer_, "pricer not set");
pricer_->initialize(*this);
rate_ = pricer_->swapletRate();
} catch (...) {
calculated_ = false;
throw;
}
}
return rate_;
}
void LazyObject::update() {
if (updating_) {
#ifdef QL_THROW_IN_CYCLES
QL_FAIL("recursive notification loop detected; you probably created an object cycle");
#else
return;
#endif
}
// This sets updating to true (so the above check breaks the
// infinite loop if we enter this method recursively) and will
// set it back to false when we exit this scope, either
// successfully or because of an exception.
UpdateChecker checker(this);
// forwards notifications only the first time
if (calculated_ || alwaysForward_) {
// set to false early
// 1) to prevent infinite recursion
// 2) otherways non-lazy observers would be served obsolete
// data because of calculated_ being still true
calculated_ = false;
// observers don't expect notifications from frozen objects
if (!frozen_)
notifyObservers();
// exiting notifyObservers() calculated_ could be
// already true because of non-lazy observers
}
} including
But can this explain the performance impact @djkrystul sees? |
I think it can't — because with the very same code and |
@pcaspers — thoughts? |
Good point. On the other hand: I saw an impact of the change by |
I see that you compute parallel shift sensi. In your case your trick works nicely. |
But now I am thinking that this will not work, i.e. if I disable notifications in settings, then bumps to quotes will not have any effect. So this approach works for you as you probably shift curves in zero space or so and not via quotes which require notifications in order to trigger recalibration, right? |
No, probably you would have to trigger recalibration manually like I "somhow" do with the What you can also try is to enable QuantLib/ql/patterns/lazyobject.hpp Lines 186 to 193 in 343ee92
|
But for me that sounds like you probably do not have a lazy |
Indeed, looks like iterative recalibration is a problem, as I said earlier, all curves recalibrate at the time, and disabling notifications is not possible in this case. I just need to check if this is indeed the case and think of some optimization if possible at all. |
So it might be the case that you even recalibrate too often, e.g. some intermediate notifications trigger a recalibration and the next forwarded one also triggers one?! |
For example, assume discounting curve G(q+s) depends on quotes q + s. And assume index (forward) curve F is a function of discounting curve G and quotes q: F( G(q+s), q). |
Ok, now I'm really confused. If I try doing that, I'm getting errors from notification cycles. Which... doesn't make sense to me at all. Any ideas as to why that might be? |
@lballabio you mean we could revert to forwarding the first notification only and instead register instrument with the evaluation date? And that would be equivalent to forwarding all notifications? |
@pcaspers that was the idea — I thought that could work, but see my later comment... |
I am not sure if that solves all possible instances. This is what I wrote above:
I don't think the global evaluation date was part of the chain A -> B -> C here. I don't remember the actual classes I was referring to here, but doesn't that show already on an abstract level that we need to always forward notifications to ensure correct behaviour? |
The idea was that the reason A and B are not calculated is almost always (yes, almost) that they're expired. When the evaluation date doesn't change, they stay expired. If the evaluation date changes, they might be alive but in 1.30 don't send a notification. If C registers with the evaluation date, A and B still won't notify, but C will receive a notification from the evaluation date and will recalculate anyway, It doesn't solve all cases, but the ones we bumped into were probably of this kind. However, it turns out that registering all instruments with the evaluation date causes notification loops to be reported. I have no idea why. |
Not sure. Think about something like Swaption -> Swap -> Coupon and a swaption engine that does not calculate the swap. The issue has nothing to do with expired instruments and evaluation date changes then. This does not match my abstract example above, since B = Swap is lazy, but it's easy to imagine a similar situation where the intermediate object is no lazy I guess. In fact I am pretty sure I saw an example for this in ORE. Anyway, shouldn't LazyObject work reliably in 100% of the cases by default? |
It's the library's way of saying "enough, enough, go for the straightforward solution!" :-) |
Regarding the loop errors: For me they start with the If I run only the So I removed tests from the suite and ended up with one test causing the error: Running the test suite without it works fine. But I have no clue why, especially why the loop is detected so much later in the suite?! test_suite* LazyObjectTest::suite() {
auto* suite = BOOST_TEST_SUITE("LazyObject tests");
suite->add(QUANTLIB_TEST_CASE(&LazyObjectTest::testDiscardingNotifications));
suite->add(QUANTLIB_TEST_CASE(&LazyObjectTest::testForwardingNotificationsByDefault));
// suite->add(QUANTLIB_TEST_CASE(&LazyObjectTest::testNotificationLoop));
return suite;
} |
A lot of But does this mean that These questions probably deserve a separate issue, unless there's a quick and easy explanation. |
Unfortunately, fossile records show that for some reason the discounting bond and swap engines set |
I have run profiler on a simple test with dual curve bootstrap where I compute par deltas of ~1000 swaps. See the screenshot below. On the left is the result with alwaysForward_ = true, on the left with alwaysForward_ = false. I will clean the code a bit and share it with you. Basically in this special example both curves recalibrate at the same time on each quote bump until they both converge, so there is no infinite loop. During this they fire up lot's of notifications. The slow down on this specific example was ~factor 10, in real application can be even worse because of many curves involved. |
Ok — when you have an example, please open a new issue. Thanks! |
Here is the code to reproduce the slow down I was talking about:
|
No description provided.