Tor Trac #25152: Try to call less circuitmux_find_map_entry() #243
Conversation
…_circuit_attached() in update_circuit_on_cmux_()
…e() take in chanid_circid_muxinfo_t
| @@ -16,6 +16,18 @@ typedef struct circuitmux_policy_s circuitmux_policy_t; | |||
| typedef struct circuitmux_policy_data_s circuitmux_policy_data_t; | |||
| typedef struct circuitmux_policy_circ_data_s circuitmux_policy_circ_data_t; | |||
|
|
|||
| /* | |||
Moving the type declarations is okay with me...
| * circuitmux_t; right now, just the count of queued cells and the direction. | ||
| */ | ||
|
|
||
| struct circuit_muxinfo_s { |
...but moving the struct definitions seems like a step backwards. Generally we try to make structure definitions more hidden, and not expose them more.
(Then again, since I am reading these commits in order, I might wind up seeing that you've done this for a very good reason later on. I'll wait and see...)
(After reading the code, I am not sure I understand why these struct definitions are exposed at all. Does anything outside circuitmux.c need to look at the members here?)
|
|
||
| /* Cmux sanity check. We check for hashent == NULL so we call | ||
| * circuitmux_find_map_entry() less. */ | ||
| if (hashent == NULL) { |
If we are calling tor_assert(hashent) above, it doesn't make sense to test it for non-NULL here. I think that the tor_assert() above must be wrong, though ... but it makes me worried that this code is not tested enough.
| @@ -561,19 +561,8 @@ circuitmux_set_policy(circuitmux_t *cmux, | |||
| */ | |||
|
|
|||
| cell_direction_t | |||
| circuitmux_attached_circuit_direction(circuitmux_t *cmux, circuit_t *circ) | |||
| circuitmux_attached_circuit_direction(chanid_circid_muxinfo_t *hashent) | |||
If we're changing the interface of this function we should change its documentation too.
| @@ -2610,7 +2610,7 @@ update_circuit_on_cmux_(circuit_t *circ, cell_direction_t direction, | |||
| file, lineno); | |||
| return; | |||
| } | |||
| tor_assert(circuitmux_attached_circuit_direction(cmux, circ) == direction); | |||
| tor_assert(circuitmux_attached_circuit_direction(hashent) == direction); | |||
One alternative might be just to remove this assertion entirely -- I don't know if that's the right choice, though.
| /* Search for this circuit's entry */ | ||
| hashent = circuitmux_find_map_entry(cmux, circ); | ||
| /* Assert that we found one */ | ||
| tor_assert(hashent); |
There's no reason to delete this assertion; and in fact we might want to have an assertion that the hashent matches cmux and circ.
| /* Okay, we need to check the circuit for policy data now */ | ||
| chanid_circid_muxinfo_t *hashent = circuitmux_find_map_entry(cmux, circ); | ||
| /* We should have found something */ | ||
| tor_assert(hashent); |
This assertion could stay -- and as before, it would be good to check this for consistency with cmux and circ.
| /* Okay, we need to check the circuit for policy data now */ | ||
| chanid_circid_muxinfo_t *hashent = circuitmux_find_map_entry(cmux, circ); | ||
| /* We should have found something */ | ||
| tor_assert(hashent); |
This assertion could stay -- and as before, it would be good to check this for consistency with cmux and circ.
| by passing in chanid_circid_muxinfo_t into functions in circuitmux.c | ||
| such as circuitmux_make_circuit_active() where a hashmap entry stays | ||
| the same across multiple subsequent function calls. Fixes bug 25152; | ||
| bugfix on 0.3.5.1-alpha. Patch by Neel Chauhan. |
When we say "Bugfix on X", we mean that we are fixing a bug, and the bug first appeared in version X. So if this performance issue first appeared in version 0.a.b.c, and we're fixing it in 0.x.y.z, then we should say "Bugfix on 0.a.b.c", not "Bugfix on 0.x.y.z".
I think we could just as easily call this particular one a feature.
Hi! I've added some questions and comments in the code. I'll leave a more general comment on the trac ticket.
| * Hash table entry (yeah, calling it chanid_circid_muxinfo_s seems to | ||
| * break the hash table code). | ||
| */ | ||
| typedef struct chanid_circid_muxinfo_t chanid_circid_muxinfo_t; |
I wonder if these types should have better names, now that we are manipulating them explicitly. The current names are pretty vague, and do not actually do a good job explaining what these types do.
| * circuitmux_t; right now, just the count of queued cells and the direction. | ||
| */ | ||
|
|
||
| struct circuit_muxinfo_s { |
(After reading the code, I am not sure I understand why these struct definitions are exposed at all. Does anything outside circuitmux.c need to look at the members here?)
For the bug here.
The text was updated successfully, but these errors were encountered: