diff mbox series

scripts: use the monotonic clock to compute time elapsed

Message ID 20230817045433.495995-1-jtilahun@astranis.com (mailing list archive)
State New
Headers show
Series scripts: use the monotonic clock to compute time elapsed | expand

Commit Message

Joseph Tilahun Aug. 17, 2023, 4:54 a.m. UTC 
From: Joseph Tilahun <jtilahun@astranis.com>

The monotonic clock is preferable over the system clock when
computing the time elapsed.

Signed-off-by: Joseph Tilahun <jtilahun@astranis.com>
---
 scripts/oe-pkgdata-browser | 4 ++--
 scripts/runqemu            | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Michael Opdenacker Aug. 17, 2023, 6:35 a.m. UTC | #1 
On 17.08.23 at 06:54, jtilahun via lists.yoctoproject.org wrote:
> From: Joseph Tilahun <jtilahun@astranis.com>
>
> The monotonic clock is preferable over the system clock when
> computing the time elapsed.


I agree. Otherwise, if you suspend your laptop, the time information 
counts the time spent sleeping.
I submitted a patch to BitBake a while ago, but there was the fear that 
my change would break other parts (though it was working fine in my tests).

However, I believe you should send your patch to 
openembedded-core@lists.openembedded.org instead, because that's a 
change for the openembedded-core repository. The "poky" repository is an 
aggregation of multiple original repositories.
Thanks!
Michael.
Richard Purdie Aug. 17, 2023, 7:29 a.m. UTC | #2 
On Thu, 2023-08-17 at 08:35 +0200, Michael Opdenacker via
lists.yoctoproject.org wrote:
> On 17.08.23 at 06:54, jtilahun via lists.yoctoproject.org wrote:
> > From: Joseph Tilahun <jtilahun@astranis.com>
> > 
> > The monotonic clock is preferable over the system clock when
> > computing the time elapsed.
> 
> 
> I agree. Otherwise, if you suspend your laptop, the time information 
> counts the time spent sleeping.
> I submitted a patch to BitBake a while ago, but there was the fear that 
> my change would break other parts (though it was working fine in my tests).

The issue was that the python manual page clearly says:

"""
The reference point of the returned value is undefined, so that only
the difference between the results of two calls is valid.
"""

and the timestamps being sent out by parts of bitbake may be used as
absolute time values (e.g. displayed to the user), not just for
comparison.

Cheers,

Richard
Joseph Tilahun Aug. 17, 2023, 8:28 p.m. UTC | #3 
Thank you for the background Michael. I sent this patch to
openembedded-core@lists.openembedded.org per your message.

Richard, I understand the point you cite from the Python manual page.
Regarding this patch specifically, only the difference between the results
of two calls is used. So the concern you raise is not applicable to this
patch specifically. Given that, I'd appreciate it if you could take a look.

Thanks,
Joseph

On Thu, Aug 17, 2023 at 12:29 AM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Thu, 2023-08-17 at 08:35 +0200, Michael Opdenacker via
> lists.yoctoproject.org wrote:
> > On 17.08.23 at 06:54, jtilahun via lists.yoctoproject.org wrote:
> > > From: Joseph Tilahun <jtilahun@astranis.com>
> > >
> > > The monotonic clock is preferable over the system clock when
> > > computing the time elapsed.
> >
> >
> > I agree. Otherwise, if you suspend your laptop, the time information
> > counts the time spent sleeping.
> > I submitted a patch to BitBake a while ago, but there was the fear that
> > my change would break other parts (though it was working fine in my
> tests).
>
> The issue was that the python manual page clearly says:
>
> """
> The reference point of the returned value is undefined, so that only
> the difference between the results of two calls is valid.
> """
>
> and the timestamps being sent out by parts of bitbake may be used as
> absolute time values (e.g. displayed to the user), not just for
> comparison.
>
> Cheers,
>
> Richard
>
>
>
Richard Purdie Aug. 17, 2023, 8:32 p.m. UTC | #4 
On Thu, 2023-08-17 at 13:28 -0700, Joseph Tilahun wrote:
> Thank you for the background Michael. I sent this patch to
> openembedded-core@lists.openembedded.org per your message.
> 
> Richard, I understand the point you cite from the Python manual page.
> Regarding this patch specifically, only the difference between the
> results of two calls is used. So the concern you raise is not
> applicable to this patch specifically. Given that, I'd appreciate it
> if you could take a look.

Alex's point stands though and relates to my concern. If we use
time.time() everywhere we're consistent and less likely to confuse the
two and get interesting results.

Is there a real world problem this change is solving? Did you have a
build machine where you changed the time part way through a build?

I'm leaning towards being consistent as this time change would seem to
be something very rare compared to the risks of people copy and pasting
and confusing the usage.

Cheers,

Richard
Joseph Tilahun Aug. 17, 2023, 8:53 p.m. UTC | #5 
The point about consistency is fair enough. time.time() is already used in
many places, so it's understandable if the desire is to continue using
time.time() throughout. I do think these usages of time in scripts are
isolated enough to where it's not as much of a concern. The time deltas are
computed purely within those functions and simply printed out, with nothing
else done with that information.

That being said, if you still insist that monotonic is not the way to go
for the sake of consistency, then fine.

Thanks,
Joseph

On Thu, Aug 17, 2023 at 1:32 PM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Thu, 2023-08-17 at 13:28 -0700, Joseph Tilahun wrote:
> > Thank you for the background Michael. I sent this patch to
> > openembedded-core@lists.openembedded.org per your message.
> >
> > Richard, I understand the point you cite from the Python manual page.
> > Regarding this patch specifically, only the difference between the
> > results of two calls is used. So the concern you raise is not
> > applicable to this patch specifically. Given that, I'd appreciate it
> > if you could take a look.
>
> Alex's point stands though and relates to my concern. If we use
> time.time() everywhere we're consistent and less likely to confuse the
> two and get interesting results.
>
> Is there a real world problem this change is solving? Did you have a
> build machine where you changed the time part way through a build?
>
> I'm leaning towards being consistent as this time change would seem to
> be something very rare compared to the risks of people copy and pasting
> and confusing the usage.
>
> Cheers,
>
> Richard
>
diff mbox series

Patch

diff --git a/scripts/oe-pkgdata-browser b/scripts/oe-pkgdata-browser
index c152c82b25..727803ba93 100755
--- a/scripts/oe-pkgdata-browser
+++ b/scripts/oe-pkgdata-browser
@@ -29,10 +29,10 @@  FileColumns = enum.IntEnum("FileColumns", {"Filename": 0, "Size": 1})
 import time
 def timeit(f):
     def timed(*args, **kw):
-        ts = time.time()
+        ts = time.monotonic()
         print ("func:%r calling" % f.__name__)
         result = f(*args, **kw)
-        te = time.time()
+        te = time.monotonic()
         print ('func:%r args:[%r, %r] took: %2.4f sec' % \
           (f.__name__, args, kw, te-ts))
         return result
diff --git a/scripts/runqemu b/scripts/runqemu
index 0e105a918b..d3c0b3fc38 100755
--- a/scripts/runqemu
+++ b/scripts/runqemu
@@ -1252,9 +1252,9 @@  to your build configuration.
         if self.snapshot and tmpfsdir:
             newrootfs = os.path.join(tmpfsdir, os.path.basename(self.rootfs)) + "." + str(os.getpid())
             logger.info("Copying rootfs to %s" % newrootfs)
-            copy_start = time.time()
+            copy_start = time.monotonic()
             shutil.copyfile(self.rootfs, newrootfs)
-            logger.info("Copy done in %s seconds" % (time.time() - copy_start))
+            logger.info("Copy done in %s seconds" % (time.monotonic() - copy_start))
             self.rootfs = newrootfs
             # Don't need a second copy now!
             self.snapshot = False