Opened 10 months ago

Closed 9 months ago

Last modified 5 months ago

#27247 closed enhancement (implemented)

Clients are using RAM for cached_dir_t

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

Description

According to the profiles, clients are storing a cached_dir_t object for the consensus they have. This is ridiculous -- they don't need that, and they already have it somewhere else.

Directory caches probably don't need one of these either -- they could mmap it instead.

Child Tickets

Change History (10)

comment:1 Changed 10 months ago by ahf

Owner: set to ahf
Status: newassigned

comment:2 Changed 10 months ago by nickm

In experiments: the cached_dir_t object, including compressed and uncompressed text, adds up to 5.7 MB out of a total of 24.7 MB used for directory allocations. If we can remove it, or replace it with something mmaped, we save about 23% of our total client memory usage.

comment:3 Changed 9 months ago by nickm

Owner: changed from ahf to nickm
Status: assignedaccepted

This turns out to be an extremely simple patch, if we try to do the simple version of this code. See branch ticket27247 ; PR at https://github.com/torproject/tor/pull/310

Teor, could you review this? It reverts part of a commit that you made in 2016, and I'm hoping you'll be able to tell me if I'm about to break something here.

I've opened #27563 for a fancier way to save this memory on directory caches. The remaining ticket (#27244) is not in fact redundant: we could use mmap here instead of read_file_to_string, but we'd need to refactor other things.

comment:4 Changed 9 months ago by nickm

Status: acceptedneeds_review

comment:5 Changed 9 months ago by nickm

Reviewer: teor

Optimistically setting the reviewer field. :)

comment:6 in reply to:  3 Changed 9 months ago by cyberpunks

Replying to nickm:

See branch ticket27247 ; PR at https://github.com/torproject/tor/pull/310

Well, I think this change looks great, at least.

comment:7 Changed 9 months ago by nickm

Type: defectenhancement

comment:8 Changed 9 months ago by teor

Status: needs_reviewmerge_ready

The original fix was #20667, I think caching consensuses in RAM on clients was a mistake in that branch.

I asked some questions on the pull request - in particular, I think this branch disables consensus diffs and if-modified-since headers on clients with FetchUselessDescriptors 1.

Even so, I would happily merge this code as-is.

(I will pull out my test bandwidth authority, and test master and this branch, but that's going to take a few days.)

comment:9 Changed 9 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

I've looked at the code and run a test, and it seems that with FetchUselessDescriptors, the client stores parsed versions of both consensus objects, so a new fallback won't be needed there. The comment about handling "a consensus we don't parse, but which we do cache" is talking about an expected future feature that we haven't implemented yet, where we teach caches to download flavors that they weren't originally programmed to know about.

Per above and per your review, I'll merge this as-is. Thanks for looking at it, and for verifying that it won't mess up #20667!

comment:10 Changed 5 months ago by nickm

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