localtime) between read() and write() copying files
ClosedPublic
Actions

Authored by jtamate on Jan 19 2018, 4:34 PM.

Details

Reviewers

dfaure
fvogt

Group Reviewers

Frameworks

Commits

R241:8d73867b3d43: Don't stat(/etc/localtime) between read() and write() copying files

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

Diff Detail

Repository

R241 KIO

Branch

timer (branched from master)

Lint

No Linters Available

Unit

No Unit Test Coverage

jtamate created this revision.Jan 19 2018, 4:34 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptJan 19 2018, 4:34 PM

jtamate requested review of this revision.Jan 19 2018, 4:34 PM

ngraham added a subscriber: ngraham.Jan 19 2018, 6:57 PM

fvogt requested changes to this revision.Jan 22 2018, 6:27 PM

fvogt added a subscriber: fvogt.

fvogt added inline comments.

src/core/slavebase.cpp
279 ↗	(On Diff #25644)	In the old code the timeout was canceled here, but now it gets started again instead.
1052 ↗	(On Diff #25644)	If I understand this correctly, this will change behaviour. It'll call the timeout function on the next dispatchLoop call.

This revision now requires changes to proceed.Jan 22 2018, 6:27 PM

jtamate added inline comments.Jan 22 2018, 7:00 PM

src/core/slavebase.cpp

279 ↗

(On Diff #25644)

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.

1052 ↗

(On Diff #25644)

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.

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.

src/core/slavebase.cpp
279 ↗	(On Diff #25644)	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.
1052 ↗	(On Diff #25644)	You probably want to call `invalidate` here as well and then not `start()` it in this case.

In 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.

src/core/slavebase.cpp
279 ↗	(On Diff #25644)	You're right. I missed that QDateTime() generates an invalid datetime.
1052 ↗	(On Diff #25644)	If you are talking only in the last case // canceled. You are right again (the same mistake).

Don't stat(/etc/localtime) between read() and write() copying files

Fix logic of cancelations

This should work now - I won't give the revision a total accept though as I didn't test it myself.

lbeltrame added a reviewer: dfaure.Jan 23 2018, 8:25 AM

dfaure requested changes to this revision.Jan 24 2018, 8:35 AM

dfaure added inline comments.

src/core/slavebase.cpp
1052	Typo: In -> Im (i.e. it was correct in the orig code)
1054	Where does this "1" value come from, and it is useful at all?
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)

This revision now requires changes to proceed.Jan 24 2018, 8:35 AM

jtamate updated this revision to Diff 25867.Jan 24 2018, 11:00 AM

jtamate marked 3 inline comments as done.

jtamate added inline comments.

src/core/slavebase.cpp
1054	It was 1 as could have been 0. It is not needed anymore.
1055	I was trying to save some code, but you're right. Here speed matters more than size.

So, good to go?

dfaure accepted this revision.Feb 7 2018, 8:32 AM

This revision is now accepted and ready to land.Feb 7 2018, 8:32 AM

Closed by commit R241:8d73867b3d43: Don't stat(/etc/localtime) between read() and write() copying files (authored by jtamate). · Explain WhyFeb 7 2018, 6:06 PM

This revision was automatically updated to reflect the committed changes.

dfaure mentioned this in D26364: SlaveBase::dispatchLoop: Fix timeout calculation.Jan 3 2020, 9:23 PM

Revision Contents
Changeset List

			Path	Packages
M			src/core/slavebase.cpp (28 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	25644	eecbc6a		Jan 19 2018, 4:34 PM	★	★
Diff 2	25783	eecbc6a	- Don't stat(/etc/localtime) between read() and write() copying files	Jan 22 2018, 8:37 PM	★	★
Diff 3	25867			Jan 24 2018, 10:59 AM	★	★
Diff 4	26709	e6611ca	R241:8d73867b3d4339ec026f92c4c2e89c265249aeb2	Feb 7 2018, 6:05 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
0a1afb0fb3aa	ff84abce712e	6056a1a32762	Jaime Torres	Don't stat(/etc/localtime) between read() and write() copying files (Show More…)	Jan 22 2018, 8:32 PM
6056a1a32762	8f186ed29166	eecbc6a083d6	Jaime Torres	Don't stat(/etc/localtime) between read() and write() copying files (Show More…)	Jan 19 2018, 4:32 PM

Diff 25783

View Options