Opened 13 months ago

Closed 5 months ago

#23354 closed enhancement (implemented)

Remove deterministic download schedule code and configs

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: refactor, s8-perf, technical-debt, tor-bootstrap, 034-triage-20180328, fast-fix
Cc: catalyst Actual Points:
Parent ID: Points: 0.1
Reviewer: dgoulet Sponsor: Sponsor8

Description (last modified by teor)

Under exponential backoff, download schedules contain the maximum time we will wait, even if the random amount is larger. initial time we will wait, and everything else is random exponential from that point onwards.

But in most cases, the random amount is much smaller than the maximum, so we could replace the item with the actual maximum, or delete it from the schedule altogether. (On the public network, the maximum is 4x the last entry, on test networks, it's 3x.)

So once we're sure that we will never revert to deterministic schedules, we should make each schedule into a single initial value, and remove the deterministic code.

We should make these changes based on the schedules in #23347.

Child Tickets

Change History (17)

comment:1 Changed 13 months ago by teor

Description: modified (diff)

comment:2 Changed 13 months ago by teor

Description: modified (diff)
Summary: Remove redundant items from download schedulesRemove deterministic download schedule code and configs

comment:3 Changed 13 months ago by nickm

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

comment:4 Changed 11 months ago by catalyst

Cc: catalyst added
Keywords: refactor s8-perf technical-debt added
Milestone: Tor: 0.3.4.x-finalTor: 0.3.3.x-final
Sponsor: Sponsor8

#24151 is a duplicate; copied text from that ticket is:

It looks like the only code that uses DL_SCHED_DETERMINISTIC is testing code. We should rip out all code related to it. This will make it easier to refactor in the future and reduce the binary size by eliminating dead code.

comment:5 Changed 8 months ago by nickm

Whoops; look like we had another duplicate as #23814. But let's keep this ticket around for just removing the configs.

comment:6 Changed 8 months ago by nickm

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

Label a bunch of (arguable and definite) enhancements as enhancements for 0.3.4.

comment:7 Changed 7 months ago by teor

Here's how we can remove these configs:

  • turn each download schedule config option into a single non-negative integer
  • set the default config value to the first item in each list
  • most schedules will be 0, so merge those schedules together, removing conditional code
  • maybe rename *DownloadSchedule to *DownloadInitialDelay

comment:8 Changed 6 months ago by nickm

Keywords: 034-triage-20180328 added

comment:9 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:10 Changed 6 months ago by nickm

Keywords: fast-fix added; 034-removed-20180328 removed
Owner: set to nickm
Status: newaccepted

comment:11 Changed 5 months ago by nickm

Status: acceptedneeds_review

As requested, this is done in ticket23354. Pull request at https://github.com/torproject/tor/pull/54

comment:12 Changed 5 months ago by teor

There is an extra commit c9c26d0 at the start of this branch.

I wonder if it is ok that our coverage dropped slightly.
But I think that's ok, because we removed 50 lines of code.

When did we stop using deterministic schedules?
I think it was 0.2.9. Do we want to say 0.2.9 in the changes file?

The rest seems sensible to me, but I would like someone else to review this too.

I opened #25844 for the relevant chutney changes.

comment:13 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

comment:14 Changed 5 months ago by nickm

Status: needs_revisionneeds_review

There is an extra commit c9c26d0 at the start of this branch.

Okay; I've rebased the branch to remove this commit.

I think it was 0.2.9. Do we want to say 0.2.9 in the changes file?

Makes sense; doing that too.

I've re-pushed the branch: there are no other changes.

The rest seems sensible to me, but I would like someone else to review this too.

Okay -- into needs_review it goes!

comment:15 Changed 5 months ago by dgoulet

Reviewer: dgoulet

comment:16 Changed 5 months ago by dgoulet

Status: needs_reviewmerge_ready

One single comment on the PR.

The rest lgtm so feel free to fix or no fix the comment on the PR and merge.

comment:17 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

You suggested, wrt getinfo_helper_config():

we might want to change this to something that does NOT say "comma list"

So, these values still accept the same format as before (ignoring everything except the first entry), and they still emit a subset of the format that they emitted before.

On the other hand, they are closer to TimeInterval now than they were before. So I'l change them to that. Done in commit b205061eb13abd68a5335f8008c5121ef230c34e.

Also, merged.

Note: See TracTickets for help on using tickets.