Performance issue

[GarnikA]

I have upgraded the QLNet from a very old version (1.0.0.1) to 1.3.0.0 and noted a huge performance drop. To give an idea: the total execution time of an application moved from 2.5 to 10 minutes. And this is ONLY upgrading the QLNet library. After spending some time, with profiling the library and comparing the code, I found that the cause of the issue is in using the Utils.QL_REQUIRE() call all over the places. Even though, it may look innocent, but having for example a date or some other data type comparison, impacts the performance, if called MANY times.
For testing purposes, I have commented out EVERY usage of Utils.QL_REQUIRE(), and the performance returned back.
I think, in a quant/math libraries should be as less "unnecessary" checks / validations. Users of the library should do some work for their own protection. We should not compromise the performance, so just to be nice to the user :)
I would like to recommend someone to remove / comment / put under some define those entries, having it disabled by default.

[igitur]

We try to keep the QLNet code in line with the C++ QuantLib project. I'll try to find out whether the additional QL_REQUIRE's had an effect on QuantLib's performance.

[GarnikA]

It must have, if called millions of times :) I am going to keep my version without those calls, have no choice. Can't compromise the performance that much. In any case thanks for looking.

[GarnikA]

Perhaps it would makes sense to modify the QL_REQUIRE, so that instead of taking a bool, it will take two parameters, and do the check inside (here you could have versions with different types, so no casting inside). Then in the function, you could use #if DEFINE_SOMETHING to perform the check. This way we can easily turn it on/off, perhaps by some build parameter. And I would set the default value for that "build parameter" to disable the check.

[igitur]

It's possible that the building of the message string has the biggest performance hit, rather than the condition statement.

The C++ project uses a macro for QL_REQUIRE, (see https://github.com/lballabio/quantlib/blob/master/QuantLib/ql/errors.hpp ), so the message will be built only if the condition holds.

Currently QLNet builds the message in any case. I think we could change the message to a lambda function to delay it.

But we can maybe include directives too, as you point out. @amaggiulli , what do you think?

[GarnikA]

Yes, you are right; the string building will definitely be big part of it. I would put both, if you ask me :) And I can do a full performance test, with our application, if you like.

[igitur]

Also, it might be best to build against git master, rather than against 1.3.0.0.

[GarnikA]

Forgot to mention, I have reproduced the same issue with the version 1.4.0.4, so yes, it make sense to put the fix to the latest.

[amaggiulli]

Thanks GarnikA for comment the performance issue , I agree with you and Francois, we have to fix. I will do some profiling to compare the different solutions .

[GarnikA]

Thanks a lot for a quick respond on this, very impressive.

[GarnikA]

I thought, you may be interested: around 85% of the slowness comes from String part, and 15% from comparison.

[igitur]

To delay the buildup of the message, one could implement a lambda function by changing QL_REQUIRE to:

       public static void QL_REQUIRE(bool condition, Func<string> message)
       {
           if (!condition)
              throw new ApplicationException(message);
       }

And changing all the calls to QL_REQUIRE like:

               Utils.QL_REQUIRE(issueDate_ < cashflows_[0].date(), () =>
                          "issue date (" + issueDate_ +
                          ") must be earlier than first payment date (" +
                          cashflows_[0].date() + ")");

It's about 550 calls that have to be changed.

I hope I'm correct in saying that this will clear up the 85% that @GarnikA is seeing.

[igitur]

Of course that would mean that QLNet won't build on early versions of .NET. I don't know what the current minimum requirement is.

[amaggiulli]

The point is the string allocation , c++ don't allocate memory if condition not meet, here we always allocate string on the heap ..Francois the lamba calling in the form Utils.QL_REQUIRE(condition, () => string); don't allocate the string anyway ? Also lambda can generate huge traffic too because each time a lambda is called, a delegate is created as well. Maybe can be usefull to share a test case for performance comparison.

[GarnikA]

Unfortunately, I don't have a ready unit test for this. Actually we upgraded the lib a few months ago, but realized the issue just now, with a specific user application. I will look into creating one, but can work on it only over the weekend. It is a thanksgiving break here. As of the fix, what do you think of just replacing this function with an actual check, then in it Throw the message, if condition is true.I would agree this is a lot changes, but I think any solution will require touching every usage of QL_REQUIRE anyway. So this will solve the string construction issue, as of the compassion, the "If" statement can be wrapped into a function, with a few overloads. I will prepare an example, to demo the idea, if I am not clear.

[GarnikA]

While improving the overall performance of our application by multi-threading, I have realized that the QLNet is not thread-safe. For example IndexManager class, has a static Dictionary data_. Is the C++ QuantLib thread-safe? Actually I have replaced the Dictionary with ConcurentDictionary and solved my immediate issue, but don't feel comfortable with that. What do you think? I can open another Issue/Conversation thread for this topic, I would agree it should not be mixed with the performance issue.

[amaggiulli]

In few mins I will commit QL_REQUIRE optimization , from my local test seems problem is solved .I'll wait for your feedback on this if you can . QLNet actually is not thread-safe , with some help we can fix that too :)

[amaggiulli]

Update master : fcef811

[igitur]

QuantLib is also not thread-safe. For example the evaluationDate is stored in a singleton.

[GarnikA]

Thanks a lot for the update. Will the "Download Zip" from https://github.com/amaggiulli/qlnet get me the latest version? I don't have the repository setup on my side :)

[amaggiulli]

yes

[GarnikA]

Just wanted to confirm, that the issue has been resolved, thanks a lot Francois and Andrea.
I am closing the issue.

[GarnikA]

Francois, on the thread-safe note; you mention that "evaluationDate is stored in a singleton". Are you referring to the static Settings class? If yes, should not [ThreadStatic] solve that problem?
From what I can see, there are only a few singletons and with very little change they can be turned to be thread-safe: IndexManager (protect the data_, actually I switch to ConcurrentDictionary and that solved the issue for me), ExchangeRateManager (already uses [ThreadStatic]) and SeedGenerator(we could just mark the instance_ with [ThreadStatic] as in the ExchangeRateManager). What do you think? It would probably make sense to have this discussion on a separate thread, I am just not sure how to create one, without entering an Issue.

[GarnikA]

Actually I think the SeedGenerator does not even need the [ThreadStatic], it is good as is.

[igitur]

I was referring to the QuantLib project, not QLNet. I know QuantLib is not thread-safe because of the singleton.