D9983: Don't stat(/etc/localtime) between read() and write() copying files
This revision was automatically updated to reflect the committed changes. Closed by commit R241:8d73867b3d43: Dont stat(/etc/localtime) between read() and write() copying files (authored by jtamate). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9983?vs=25867=26709 REVISION DETAIL https://phabricator.kde.org/D9983 AFFECTED FILES src/core/slavebase.cpp To: jtamate, #frameworks, dfaure Cc: fvogt, ngraham, michaelh
D9983: Don't stat(/etc/localtime) between read() and write() copying files
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9983 To: jtamate, #frameworks, dfaure Cc: fvogt, ngraham, michaelh
D9983: Don't stat(/etc/localtime) between read() and write() copying files
jtamate marked 2 inline comments as done. jtamate added a comment. So, good to go? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9983 To: jtamate, #frameworks, dfaure Cc: fvogt, ngraham, michaelh
D9983: Don't stat(/etc/localtime) between read() and write() copying files
jtamate marked 3 inline comments as done. jtamate added inline comments. INLINE COMMENTS > dfaure wrote in slavebase.cpp:1054 > Where does this "1" value come from, and it is useful at all? It was 1 as could have been 0. It is not needed anymore. > dfaure wrote in slavebase.cpp:1055 > So when we go to that "else" branch, we'll have called start() and > immediately after, invalidate(). To avoid doing this, how about calling > start() in the first two if()s? > (small optimization) I was trying to save some code, but you're right. Here speed matters more than size. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9983 To: jtamate, #frameworks, dfaure Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
jtamate updated this revision to Diff 25867. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9983?vs=25783=25867 REVISION DETAIL https://phabricator.kde.org/D9983 AFFECTED FILES src/core/slavebase.cpp To: jtamate, #frameworks, dfaure Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> slavebase.cpp:1052
> } else if (timeout == 0) {
> -d->nextTimeout = QDateTime::currentDateTime().addSecs(1);//
> Immediate timeout
> +d->nextTimeoutMsecs = 1000; // Inmediate timeout
> } else {
Typo: In -> Im (i.e. it was correct in the orig code)
> slavebase.cpp:1054
> } else {
> -d->nextTimeout = QDateTime();// Canceled
> +d->nextTimeoutMsecs = 1;
> +d->nextTimeout.invalidate(); // Canceled
Where does this "1" value come from, and it is useful at all?
> slavebase.cpp:1055
> +d->nextTimeoutMsecs = 1;
> +d->nextTimeout.invalidate(); // Canceled
> }
So when we go to that "else" branch, we'll have called start() and immediately
after, invalidate(). To avoid doing this, how about calling start() in the
first two if()s?
(small optimization)
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9983
To: jtamate, #frameworks, dfaure
Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
lbeltrame added a reviewer: dfaure. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9983 To: jtamate, #frameworks, dfaure Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
fvogt resigned from this revision. fvogt added a comment. This should work now - I won't give the revision a total accept though as I didn't test it myself. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9983 To: jtamate, #frameworks Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
jtamate updated this revision to Diff 25783. jtamate added a comment. - Don't stat(/etc/localtime) between read() and write() copying files Fix logic of cancelations REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9983?vs=25644=25783 BRANCH timer (branched from master) REVISION DETAIL https://phabricator.kde.org/D9983 AFFECTED FILES src/core/slavebase.cpp To: jtamate, #frameworks, fvogt Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
jtamate added a comment. In https://phabricator.kde.org/D9983#194505, @fvogt wrote: > Just a quick idea, it might be wrong: If you use a `QTimer` instead of `QElapsedTimer`, you can get rid of `nextTimeoutMsecs`. This would also simplify the logic a bit. QTimer and I have a strange relationship and I try to avoid it. But feel free to implement it that way. INLINE COMMENTS > fvogt wrote in slavebase.cpp:279 > This does not matter - previously this line guaranteed that this does not > fire again until `nextTimeout` is set again. Now it will fire on the next > `dispatchLoop()` call. > > You probably want to call `d->nextTimeout.invalidate()` here. You're right. I missed that QDateTime() generates an invalid datetime. > fvogt wrote in slavebase.cpp:1052 > You probably want to call `invalidate` here as well and then not `start()` it > in this case. If you are talking only in the last case // canceled. You are right again (the same mistake). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9983 To: jtamate, #frameworks, fvogt Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
fvogt added a comment. Just a quick idea, it might be wrong: If you use a `QTimer` instead of `QElapsedTimer`, you can get rid of `nextTimeoutMsecs`. This would also simplify the logic a bit. INLINE COMMENTS > jtamate wrote in slavebase.cpp:279 > Reading the documentation http://doc.qt.io/qt-5/qelapsedtimer.html > I'm not able to say if start() will restart a timer or continue. My guess is > that it also restarts. > And also because calling restart() on a QElapsedTimer that is invalid results > in undefined behavior. This does not matter - previously this line guaranteed that this does not fire again until `nextTimeout` is set again. Now it will fire on the next `dispatchLoop()` call. You probably want to call `d->nextTimeout.invalidate()` here. > jtamate wrote in slavebase.cpp:1052 > | before | now| where > | > | next = now + timeout | msecs = timeout, (re)start elapsed | > setTimeoutSpecialCommand | > | if next < now then | if has elapsed timeout then | dispatchLoop > | > | > > I think both are equivalent. You probably want to call `invalidate` here as well and then not `start()` it in this case. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9983 To: jtamate, #frameworks, fvogt Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
jtamate added inline comments. INLINE COMMENTS > fvogt wrote in slavebase.cpp:279 > In the old code the timeout was canceled here, but now it gets started again > instead. Reading the documentation http://doc.qt.io/qt-5/qelapsedtimer.html I'm not able to say if start() will restart a timer or continue. My guess is that it also restarts. And also because calling restart() on a QElapsedTimer that is invalid results in undefined behavior. > fvogt wrote in slavebase.cpp:1052 > If I understand this correctly, this will change behaviour. > It'll call the timeout function on the next dispatchLoop call. | before | now| where | | next = now + timeout | msecs = timeout, (re)start elapsed | setTimeoutSpecialCommand | | if next < now then | if has elapsed timeout then | dispatchLoop | | I think both are equivalent. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9983 To: jtamate, #frameworks, fvogt Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> slavebase.cpp:279
> QByteArray data = d->timeoutData;
> -d->nextTimeout = QDateTime();
> +d->nextTimeout.start();
> d->timeoutData = QByteArray();
In the old code the timeout was canceled here, but now it gets started again
instead.
> slavebase.cpp:1052
> } else {
> -d->nextTimeout = QDateTime();// Canceled
> +d->nextTimeoutMsecs = 1; // Canceled
> }
If I understand this correctly, this will change behaviour.
It'll call the timeout function on the next dispatchLoop call.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9983
To: jtamate, #frameworks, fvogt
Cc: fvogt, ngraham
D9983: Don't stat(/etc/localtime) between read() and write() copying files
jtamate created this revision. jtamate added a reviewer: Frameworks. Restricted Application added a project: Frameworks. jtamate requested review of this revision. REVISION SUMMARY CCBUG: 384561 Unfortunately, QDateTime::currentDateTime() checks /etc/localtime each time it is called. Chaning to QElapsedTime, no check of /etc/localtime. Reproducing bug 384561, the strace of file.so was something like: read(), stat(/etc/localtime), stat(/etc/localtime), stat(/etc/localtime), stat(/etc/localtime), stat(/etc/localtime), write(), read() .. Now it is: read(), write() It also reduces the cpu in io/wait around 10% in a debug build. TEST PLAN kio tests work as before desktop: works in dolphin REPOSITORY R241 KIO BRANCH timer (branched from master) REVISION DETAIL https://phabricator.kde.org/D9983 AFFECTED FILES src/core/slavebase.cpp To: jtamate, #frameworks
