#24374 closed enhancement (implemented)

Reduce monotime_coarse_absolute_msec() usage

Reported by: ahf Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: s8-perf, review-group-27
Cc: Actual Points:
Parent ID: #24062 Points:
Reviewer: teor Sponsor: Sponsor8


monotime_coarse_absolute_msec() is making a lot of calls to the compiler generated function __udivdi3 to the point where it pops up in our profiling on Android.

We should reduce this usage and ensure the high amount of calls to __udivdi3 goes away.

Child Tickets

Change History (11)

comment:1 Changed 20 months ago by nickm

Milestone: Tor: 0.3.3.x-final
Owner: set to nickm
Status: newaccepted

This shouldn't be hard. The trick will be to use monotime_coarse_t in more places, and only convert to msec as lazily as possible.

comment:2 Changed 20 months ago by nickm

Note to self -- I've started work here in a branch called coarse_monotime_032. Right now the unit tests are broken and it wastes a bunch of RAM.

I've also just thought of a better way: monotime_coarse_stamps has the key concept, though it isn't finished.

comment:3 Changed 20 months ago by nickm

I believe monotime_coarse_stamps is ready for review, though I want to double-check the numer/denom stuff for osx.

As a followup ticket, we should look into using this for the padding timers as well.

comment:4 Changed 20 months ago by nickm

Status: acceptedneeds_review

comment:5 Changed 20 months ago by nickm

Keywords: review-group-27 added

comment:6 Changed 20 months ago by teor

Reviewer: teor

This compiles and passes make check for me on macOS 10.12, x86_64, Apple clang 9.0.0.
I haven't tested it on i386, because I don't have the libraries installed on my local machine.

I checked that mach_time_info.numer and denom are around the right way.

The quotient can be up to 40 on PowerPC macs, see:

I think we are safe from overflow here. And in this context, the divide then multiply inaccuracy doesn't matter.

Next step is to check on i386 and mobile.

comment:7 Changed 20 months ago by nickm

I'd be okay with taking this before we test it on hardware we don't have, and letting the integration tests let us know if we missed anything... so long as the code looks okay?

comment:8 Changed 20 months ago by teor

Status: needs_reviewmerge_ready

The code looks fine, lgtm.

comment:9 Changed 20 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

then, merged! thanks for the review and the testing!

comment:10 Changed 19 months ago by nickm

Resolution: fixed
Status: closedreopened

Note: this is missing a changes file!

comment:11 Changed 19 months ago by nickm

Resolution: implemented
Status: reopenedclosed

Changes file added as 8441189b3caee2

Note: See TracTickets for help on using tickets.