Opened 13 months ago
Closed 13 months ago
#25163 closed defect (implemented)
rephist: Remove unused counters in or_history_t
Reported by: | dgoulet | Owned by: | dgoulet |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: 0.3.3.x-final |
Component: | Core Tor/Tor | Version: | |
Severity: | Normal | Keywords: | tor-rephist, code-removal |
Cc: | karsten | Actual Points: | |
Parent ID: | Points: | ||
Reviewer: | Sponsor: |
Description
The rephist subsystem seems to track EXTEND cells attempt (on the client side) but the overall results of this is not used except in rep_hist_dump_stats()
. This is the only use of link_history_t
also afaict. See:
void rep_hist_note_extend_succeeded(const char *from_name, const char *to_name); void rep_hist_note_extend_failed(const char *from_name, const char *to_name);
Furthermore, tor tracks downtime and uptime of relays but actually never use that information anywhere. See:
void rep_hist_note_connect_failed(const char* nickname, time_t when); void rep_hist_note_connect_succeeded(const char* nickname, time_t when); void rep_hist_note_disconnect(const char* nickname, time_t when); void rep_hist_note_connection_died(const char* nickname, time_t when);
The side effect of this is that we keep adding objects to the history_map
that are wasting memory and never used in the end except when we dump statistics.
By removing this, we would cleanup quite a bit of code and effectively make the or_history_t
object *ONLY* useful to directory authorities for relay reachability tracking.
Child Tickets
Change History (9)
comment:1 Changed 13 months ago by
Status: | new → needs_review |
---|
comment:2 Changed 13 months ago by
Status: | needs_review → merge_ready |
---|
I'm okay with these. I'd like others (arma? karsten?) to get a chance to sign off on them first, though.
comment:3 Changed 13 months ago by
Cc: | karsten added |
---|
comment:4 Changed 13 months ago by
It looks like this change won't affect anything related to metrics. But I have no idea whether people would miss these stats from the SIGUSR1 signal. Maybe this can be useful information to debug something? Maybe ask on tor-relays@?
comment:5 Changed 13 months ago by
Owner: | set to dgoulet |
---|---|
Status: | merge_ready → accepted |
comment:6 Changed 13 months ago by
Status: | accepted → merge_ready |
---|
comment:7 Changed 13 months ago by
i haven't looked at the patch, but dgoulet filed this "why not get this of this stuff" ticket after talking to me on irc, so i am ok with the concept.
comment:8 Changed 13 months ago by
8 files changed, 11 insertions(+), 331 deletions(-)
Merged to master!
comment:9 Changed 13 months ago by
Resolution: | → implemented |
---|---|
Status: | merge_ready → closed |
This removes some stats from a SIGUSR1 signal but it also reduces considerably the size of the or history map in rephist.c
If you think we should keep this for the stats that it gives us, feel free to toss the branch out.
See branch:
ticket25163_033_01