Opened 3 months ago

Closed 2 months ago

#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 3 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 3 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 3 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 3 months ago by nickm

Status: acceptedneeds_review

comment:5 Changed 3 months ago by nickm

Keywords: review-group-27 added

comment:6 Changed 3 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 3 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 3 months ago by teor

Status: needs_reviewmerge_ready

The code looks fine, lgtm.

comment:9 Changed 3 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

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

comment:10 Changed 2 months ago by nickm

Resolution: fixed
Status: closedreopened

Note: this is missing a changes file!

comment:11 Changed 2 months ago by nickm

Resolution: implemented
Status: reopenedclosed

Changes file added as 8441189b3caee2

Note: See TracTickets for help on using tickets.