Opened 9 months ago

Closed 7 months ago

Last modified 5 months ago

#27225 closed defect (implemented)

Perform fewer allocations in summarize_protocol_flags()

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 035-roadmap-master, 035-triaged-in-20180711
Cc: Actual Points: .3
Parent ID: #26630 Points:
Reviewer: mikeperry Sponsor: Sponsor8

Description

According to our profiles, summarize_protocol_flags() does a huge number of allocations -- probably because it parses the same flags over and over.

Probably it would be simplest just to memoize the output.

Child Tickets

Change History (11)

comment:1 Changed 8 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Deferring various feature-y things to 0.3.6. If one of these is actually happening in 0.3.5, please let me know!

comment:2 Changed 7 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 7 months ago by nickm

Status: acceptedneeds_review

See branch ticket27225 with PR at https://github.com/torproject/tor/pull/422

comment:4 Changed 7 months ago by dgoulet

Reviewer: mikeperry

comment:5 Changed 7 months ago by mikeperry

Hrmm question: Do we require a specific ordering on protover keywords? The proposal says we "should" sort by keyword. But if other tor implementations do not carefully order protover strings (or if modularization of core-tor makes the protover strings dynamically generated in non-deterministic order), then we may exceed MAX_PROTOVER_SUMMARY_MAP_LEN, right?

comment:6 Changed 7 months ago by nickm

We do not require such an ordering.

It's intentional that we can exceed MAX_PROTOVER_SUMMARY_MAP_LEN -- the outcome there is just a performance regression, though, rather than any kind of exhaustion.

We could start enforcing this ordering at the authorities if we want: this would take a new proposal. Do you think it's worth it?

comment:7 Changed 7 months ago by mikeperry

Status: needs_reviewmerge_ready

Yeah I suppose we can deal with the ordering issue if/when different implementations/modularization actually becomes an issue.

comment:8 Changed 7 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

okay, squashed and merged!

comment:9 Changed 7 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:10 Changed 6 months ago by nickm

There was a lingering issue here, as bug #28558

comment:11 Changed 5 months ago by nickm

Actual Points: .3
Note: See TracTickets for help on using tickets.