Skip to content
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

fix QDateTime performance on Qt6 #323

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

alexagr
Copy link

@alexagr alexagr commented Jan 22, 2024

Switching from Qt 5.15.2 to Qt6.5.1 on Windows platform introduces HUGE performance regression in parsing of date fields when opening the XLSX files. On my laptop (i7-10510U, 32 GB RAM, Windows 10 Pro) opening of 2 MB file takes roughly 3 sec on Qt 5.15.2 and more than 3 minutes (!) on Qt 6.5.1.

The change seems to be related to some underlying changes in QDateTime class implementation - specifically performance of copy constructors seems to have degraded a lot in Qt6.

I was able to solve the problem by switching implementation of datetimeFromNumber() to new QDateTime::fromMSecsSinceEpoch() function introduced in Qt6.5.

@dantti
Copy link
Member

dantti commented Apr 1, 2024

You are likely using a debug relase of Qt which indeed see degraded perfomance, still I don't like the ifdefs as this could be used byt Qt5 and 6, also it seems more interesting to have a QDateTime static than qint64 especially since we should be using std::chrono where possible

@alexagr
Copy link
Author

alexagr commented Apr 1, 2024

This happens with Qt 6.5.1 in release (not debug) build using MSVC 2019 compiler.

@archibalduk
Copy link

This might be related to the issue and suggested fix I posted here: #322

@dantti
Copy link
Member

dantti commented Jul 3, 2024

@dfaure-kdab you asked about a PR related to QDateTime, I know you know this issue better, and I didn't have time to dig much into this, do you think this fix is ok?

@dfaure-kdab
Copy link
Contributor

This fix looks OK, actually. I just wonder why the version check says > 6.5 while this API exists in 6.0 already. It seems to me it could be changed to say QT_VERSION_CHECK(6, 0, 0) instead, no?

@dantti dantti merged commit f5d0795 into QtExcel:master Sep 2, 2024
27 checks passed
@dantti
Copy link
Member

dantti commented Sep 2, 2024

quite confusing because docs says it was added with 6.5 https://doc.qt.io/qt-6/qdatetime.html#fromMSecsSinceEpoch-2 but older versions Qt5 included also had it with other signatures that were deprecated...

@dfaure-kdab
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants