Cache UTC offset #3898
Cache UTC offset #3898
Conversation
- avoid at least part of the costly operations
|
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
src/core/StelCore.hpp
Outdated
| double getUTCOffset(const double JD) const; | ||
| private: | ||
| // Cache is over qHash(&UTCOffsetHashData). | ||
| static QCache<size_t, qint64> utcOffsetCache; |
There was a problem hiding this comment.
If I make this a per-object variable, it breaks many const-qualified functions. If there will ever be more than one core, they would just share the results. Or do you see problems with this?
There was a problem hiding this comment.
If I make this a per-object variable, it breaks many const-qualified functions.
This is exactly why mutable keyword was introduced: one of the uses for it is implementation of transparent caches.
If there will ever be more than one core, they would just share the results.
Hmm, this doesn't seem like a likely situation, but in this way the staticness might make sense, though there's some strangeness in the way you implement the cache that I can't tell whether it actually makes sense.
There was a problem hiding this comment.
OK, I forgot about mutable.
| qint64 shiftInSeconds = 0; | ||
| if (tzName=="system_default" || (loc.planetName=="Earth" && !tzValid && !QString("LMST LTST").contains(tzName))) | ||
| // Complete the cache object and hash it. | ||
| u.loc= & const_cast<StelLocation &>(loc); |
There was a problem hiding this comment.
This is some ugliness here. Why do you keep the object by const reference and then violate this constness?
There was a problem hiding this comment.
This is what various warnings made me do.
There was a problem hiding this comment.
Well, this is the wrong way to read the warnings then :P They are supposed to make you improve the code, not equip it with kludges.
| StelUtils::getDateTimeFromJulianDay(JD, &year, &month, &day, &hour, &minute, &second); | ||
| // This method takes a significant amount of time. Try to cache a few entries. | ||
| // All these elements can influence UTCOffset, so we must include them in the cache. | ||
| // Presumably, offset does not change during a minute, so we can exclude seconds here. |
There was a problem hiding this comment.
Looks like an unsafe assumption to me. What is the cost of keeping the seconds?
There was a problem hiding this comment.
updates every second, not every minute.
|
What is the performance benefit from this? Is it even measurable in any way except by using a profiler? |
I just wondered what takes so long just displaying InfoText. The commonInfoString was a surprisingly costly part of it, and here this seemingly trivial UTC lookup appears to be far from trivial. |
| // Complete the cache object and hash it. | ||
| u.loc= & const_cast<StelLocation &>(loc); | ||
| u.tz = & const_cast<QTimeZone&>(tz); | ||
| size_t dateLocHash=qHash(&u); |
There was a problem hiding this comment.
Are you hashing a pointer to a local variable?..
There was a problem hiding this comment.
Did I misunderstand the qHash() docs? Does this only hash the pointer, not its contents?
OK, that's not what I wanted... :-o
Do I need to write my own qHash function for the struct?
There was a problem hiding this comment.
Yes, you need a custom hash function for every custom data type. This may be of help.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Finding UTC offset is surprisingly costly but is executed per-frame, shown in VTune. Caching the result per-minute saves at least part of it.
This needs some testing.
Description
Fixes # (issue)
Screenshots (if appropriate):
With this caching:
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: