Opened 5 months ago

Closed 4 months ago

#27224 closed enhancement (implemented)

Call node_get_all_orports() less from node_is_a_configured_bridge()

Reported by: nickm Owned by: rl1987
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:
Parent ID: #26630 Points:
Reviewer: teor Sponsor: Sponsor8

Description

According to our profile, this function accounts for a huge amount of the total calls to malloc() that we do. Although I don't think it accounts for much allocation at any given time, it is probably slowing us down.

Child Tickets

Change History (14)

comment:1 Changed 5 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:2 Changed 5 months ago by rl1987

The patch could be something like this: https://github.com/rl1987/tor/commit/cf9a7a8e8f82bc769220f5859ef00e3e9a796654

However, for some reason this makes entrynodes/node_filter fail. Couldn't figure out yet. Can anyone spot the key difference between old code and new code?

comment:3 Changed 5 months ago by rl1987

Status: acceptedneeds_review

Okay, I got it done and ready for review. https://github.com/torproject/tor/pull/293

comment:4 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

This branch fails Linux gcc with:

gcc -std=gnu99 -DHAVE_CONFIG_H -I.  -I./src -I./src/ext -I./src/ext/trunnel -I./src/trunnel -I./src/ext -Isrc/ext -DSHARE_DATADIR="\"/usr/local/share\"" -DLOCALSTATEDIR="\"/usr/local/var\"" -DBINDIR="\"/usr/local/bin\"" -DTOR_UNIT_TESTS  -DHAVE_MODULE_DIRAUTH=1  -ftrapv -fsanitize=address      -g -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wstack-protector --param ssp-buffer-size=1 -fPIE -fno-omit-frame-pointer -fasynchronous-unwind-tables -Wall -fno-strict-aliasing -Waddress -Warray-bounds -Wdouble-promotion -Wextra -Winit-self -Wlogical-op -Wmissing-field-initializers -Wmissing-format-attribute -Wmissing-noreturn -Wnormalized=nfkc -Woverlength-strings -Woverride-init -Wshadow -Wstrict-overflow=1 -Wsuggest-attribute=format -Wsuggest-attribute=noreturn -Wsync-nand -Wtrampolines -Wunused-but-set-parameter -Wunused-but-set-variable -Wunused-local-typedefs -Wvariadic-macros -W -Wfloat-equal -Wundef -Wpointer-arith -Wstrict-prototypes -Wmissing-prototypes -Wwrite-strings -Wredundant-decls -Wchar-subscripts -Wcomment -Wformat=2 -Wwrite-strings -Wnested-externs -Wbad-function-cast -Wswitch-enum -Waggregate-return -Wpacked -Wunused -Wunused-parameter  -Wold-style-definition -Wmissing-declarations -Werror -MT src/feature/dircache/src_core_libtor_app_testing_a-directory.o -MD -MP -MF src/feature/dircache/.deps/src_core_libtor_app_testing_a-directory.Tpo -c -o src/feature/dircache/src_core_libtor_app_testing_a-directory.o `test -f 'src/feature/dircache/directory.c' || echo './'`src/feature/dircache/directory.c
src/feature/client/bridges.c: In function ‘bridge_exists_with_ipv4h_addr_and_port’:
src/feature/client/bridges.c:291:3: error: missing initializer for field ‘family’ of ‘tor_addr_t’ [-Werror=missing-field-initializers]
   tor_addr_t node_ipv4 = {};
   ^
In file included from ./src/core/or/or.h:51:0,
                 from src/feature/client/bridges.c:16:
./src/lib/net/address.h:69:15: note: ‘family’ declared here
   sa_family_t family;
               ^
cc1: all warnings being treated as errors

I believe the correct syntax for zero-initialising any number of struct members is:

  tor_addr_t node_ipv4 = {0};

But it's slightly more correct to do:

  tor_addr_t node_ipv4;
  tor_addr_make_unspec(&node_ipv4, AF_INET);

comment:6 in reply to:  4 Changed 5 months ago by cyberpunks

Replying to teor:

But it's slightly more correct to do:

  tor_addr_t node_ipv4;
  tor_addr_make_unspec(&node_ipv4, AF_INET);

Could also just move the declaration inside the if statement, right before the tor_addr_from_ipv4h() call, right?

comment:7 Changed 5 months ago by rl1987

Status: needs_revisionneeds_review

Interesting - it compiled alright in my Vagrant box. Pushed some fixup commits that removed the offending syntax.

comment:8 Changed 5 months ago by teor

Status: needs_reviewmerge_ready

Thanks! Looks fine to me.

comment:9 Changed 5 months ago by nickm

Status: merge_readyneeds_revision

I don't think this will actually return the same results as the old function.

In the case when the bridge has no identity set, but where the node does have an identity (*), this code will always return "false" -- but the old code would check the addresses in that case.

There may be other cases when the outputs are different; I'm not sure that I spotted them all.

(*) I think the node will always have an identity, in fact.

comment:10 Changed 5 months ago by nickm

Here's the behavior that I think we had before, and which I think we want:

  • A node will match every bridge_info_t with the same identity.
  • A node will match every bridge_info_t with no identity set, if that bridge_info_t has the same address in the routerinfo, the routerstatus, or the microdescriptor.

comment:11 Changed 5 months ago by rl1987

Status: needs_revisionneeds_review

Pushed one more commit that hopefully restores the old behaviour: https://github.com/rl1987/tor/commit/2e142195a0180bdd2d9354c14e53f03180359840

Please review.

comment:12 Changed 4 months ago by asn

Reviewer: teor

comment:13 Changed 4 months ago by nickm

Type: defectenhancement

comment:14 Changed 4 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

To test this, when I squashed the branch, I tried taking only the unit tests, with none of the bridges.c changes, and making sure that they passed on the old bridges.c code as well as the new. They did! So, that's good.

But then I was worried that you hadn't tested the case where a bridge had a matching address but a mismatched digest. In fact, I thought it wouldn't work, since I had forgotten how get_configured_bridge_by_addr_port_digest actually behaved... So I added a test for that too -- and the code still passes before and after.

Nicely done! I think this code is correct.

I'm merging bug27224_take2_squashed to master now. It's the same as your branch, squashed and rearranged, with an extra test.

Note: See TracTickets for help on using tickets.