Opened 9 months ago

Closed 5 months ago

#24688 closed defect (implemented)

timing wheels should use 32-bit math on 32-bit platforms

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: cpu, 32-bit, timing, 034-triage-20180328, fast-fix
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor: Sponsor8-can

Description

I think this might help our 32-bit performance a bit.

Not putting this in needs_review yet, since it needs more analysis.

diff --git a/src/common/timers.c b/src/common/timers.c
index 93cde7de5fbd4b..3c806b7f4b422a 100644
--- a/src/common/timers.c
+++ b/src/common/timers.c
@@ -66,6 +66,11 @@ struct timeout_cb {
  * above TIMEOUT_MAX can also be super-inefficent. Choosing 5 here sets
  * timeout_max to 2^30 ticks, or 29 hours with our value for USEC_PER_TICK */
 #define WHEEL_NUM 5
+#if SIZEOF_VOID_P == 4
+/* On 32-bit platforms, we want to override wheel_bit, so that timeout.c will
+ * use 32-bit math. */
+#define WHEEL_BIT 5
+#endif
 #include "src/ext/timeouts/timeout.c"
 
 static struct timeouts *global_timeouts = NULL;

Child Tickets

Change History (8)

comment:1 Changed 8 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Mark a lot of assigned/needs_revision tickets as 0.3.4. If you think this should happen in 0.3.3 instead, just let me know?

comment:2 Changed 6 months ago by nickm

Keywords: 034-triage-20180328 added

comment:3 Changed 6 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:4 Changed 6 months ago by nickm

Keywords: fast-fix added; 034-removed-20180328 removed

comment:5 Changed 6 months ago by nickm

Status: assignedneeds_review

So, I've moved this into a branch called bug24688. I've tested it enough to make sure it's not totally broken, but I can't vouch for whether the performance improvement is actually visible on 32-bit platforms or not.

I think it might be okay to merge anyway, since this change can't make performance _worse_.

comment:6 Changed 5 months ago by asn

Reviewer: asn

comment:7 Changed 5 months ago by asn

Status: needs_reviewmerge_ready

Patch seems like an OK change if we have hints it can improve performance. However, I didn't manage to find any other tickets with profiler outputs etc.

Anyhow, I guess the most important thing is to check for correctness. I couldn't do that with code review because the timeouts.c codebase is very intense. However, I did the following:

  • Read the timeouts documentation and ensured that WHEEL_BIT is configurable. Also we are already configuring WHEEL_NUM.
  • Cloned official timeouts codebase from https://github.com/wahern/timeout and changed the WHEEL_BIT variable and ran the unittests. Passed fine.
  • Tested that the WHEEL_BIT variable gets set correctly in the Tor codebase when the #ifdef passes.
  • Tested that a Tor client works fine with WHEEL_BIT set to 5.

I think that looks good enough.

comment:8 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master!

Note: See TracTickets for help on using tickets.