Opened 14 months ago

Closed 5 months ago

#23094 closed enhancement (implemented)

prop224: Optimize hsdir_index calculation

Reported by: asn Owned by: neel
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, prop224-extra, 032-unreached
Cc: neel@… Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description

Let's include hsdir_index_t in the node_t instead of a pointer that we allocate, since that structure is always present anyway.

See here for more details:
https://oniongit.eu/dgoulet/tor/merge_requests/6#note_412

Child Tickets

Attachments (1)

b23094-r001.patch (10.6 KB) - added by neel 5 months ago.
Patch (Revision 1)

Download all attachments as: .zip

Change History (13)

comment:1 Changed 13 months ago by dgoulet

Parent ID: #20657

comment:2 Changed 13 months ago by asn

Summary: prop224: Include hsdir_index in node_t instead of pointerprop224: Optimize hsdir_index calculation

comment:3 Changed 13 months ago by asn

After our #23387 refactoring of hsdir index logic Nick suggested that we don't need to keep all 3 around in memory, since two of them are always identical:
https://oniongit.eu/asn/tor/merge_requests/6#note_1201

comment:4 Changed 12 months ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark a large number of tickets that I do not think we will do for 0.3.2.

comment:5 Changed 5 months ago by neel

Cc: neel@… added
Owner: set to neel
Status: newassigned

Changed 5 months ago by neel

Attachment: b23094-r001.patch added

Patch (Revision 1)

comment:6 Changed 5 months ago by neel

I have a patch for this. The filename is b23094-r001.patch.

comment:7 Changed 5 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Status: assignedneeds_review
Type: defectenhancement

Seems ok to me, but someone should turn it into a GitHub branch and run it through Travis CI before it is merged.

Neel, if you would like to run your branches through Travis CI, here is how to set it up:
https://trac.torproject.org/projects/tor/ticket/23883#comment:3

comment:8 Changed 5 months ago by neel

Travis CI for this patch is: https://travis-ci.org/neelchauhan/tor/builds/372561294

It seems to have all passed.

comment:9 in reply to:  3 ; Changed 5 months ago by teor

Status: needs_reviewmerge_ready

Replying to asn:

Let's include hsdir_index_t in the node_t instead of a pointer that we allocate, since that structure is always present anyway.

See here for more details:
https://oniongit.eu/dgoulet/tor/merge_requests/6#note_412

This is implemented in https://github.com/neelchauhan/tor/tree/b23094 and is ready to merge.

Replying to asn:

After our #23387 refactoring of hsdir index logic Nick suggested that we don't need to keep all 3 around in memory, since two of them are always identical:
https://oniongit.eu/asn/tor/merge_requests/6#note_1201

Removing hsdir_index_t.fetch is not implemented.
We need to:

  • analyse the state machine of fetch, store_first, and store_second to make sure that fetch is always equal to store_first or store_second
  • work out the condition that we can use to select betweeen store_first or store_second when we want fetch
  • write a function that produces fetch from a hsdir_index_t, and use it instead of fetch

We can do this in a separate ticket if you'd like to merge just the first patch. They are independent. And integers are cheap.

comment:10 in reply to:  9 Changed 5 months ago by neel

Replying to teor:

We can do this in a separate ticket if you'd like to merge just the first patch. They are independent. And integers are cheap.

I think think we should just merge.

comment:11 Changed 5 months ago by teor

Ok, I opened #24964 for the remaining work.

comment:12 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged this to master; thanks!

Note: See TracTickets for help on using tickets.