Clients are using RAM for cached_dir_t

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.

changed milestone to %Tor: 0.3.5.x-final

added 035-roadmap-master 035-triaged-in-20180711 actualpoints::.5 component::core tor/tor milestone::Tor: 0.3.5.x-final owner::nickm parent::27243 priority::medium resolution::implemented reviewer::teor severity::normal sponsor::8 status::closed type::enhancement labels

Trac:
Status: new to assigned
Owner: N/A to ahf

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.

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 (moved) for a fancier way to save this memory on directory caches. The remaining ticket (#27244 (moved)) is not in fact redundant: we could use mmap here instead of read_file_to_string, but we'd need to refactor other things.

Trac:
Status: assigned to accepted
Owner: ahf to nickm

Trac:
Status: accepted to needs_review

Optimistically setting the reviewer field. :)

Trac:
Reviewer: N/A to teor

Replying to nickm:

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

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

Trac:
Username: cyberpunks

Trac:
Type: defect to enhancement

The original fix was #20667 (moved), 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.)

Trac:
Status: needs_review to merge_ready

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 (moved)!

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

Trac:
Actualpoints: N/A to .5

closed

added 4h of time spent

mentioned in issue #27629 (moved)

mentioned in issue #27629 (moved)

moved to tpo/core/tor#27247 (closed)