Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#12859 closed enhancement (fixed)

Improve parsing speed with profile-directed code tuning

Reported by: nickm Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by atagar)

Hi! I took a quick look at how Stem parses descriptors, and tried running cProfile over a networkstatus parse. I don't think I have all of the issues solved, but I managed to get a few of the hot spots calmed down for a speed reduction of about 30%.

I don't promise that these are all correct, though I am pretty sure that the first one is a big win that you should take. It accounts for most of the speedup that I'm seeing.

Thanks again for the patches!

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by nickm

Actually, you could tighten up the new regex in 0002 even more. In my profiles, it's faster still to use:

match_re = re.compile(r'^(%s)(?:[ \t]|$)' % "|".join(keywords))

comment:2 Changed 3 years ago by nickm

  • Status changed from new to needs_review

comment:3 Changed 3 years ago by atagar

  • Status changed from needs_review to needs_revision

Hi Nick, these are great! First patch in particular is fantastic (faster *and* drops unnecessarily convoluted code, hot!). The others however break the unit tests. You should be able to easily repro this with 'run_tests.py --unit'...

https://stem.torproject.org/faq.html#how-do-i-run-the-tests

I owe a couple other people code reviews but I'll go ahead and correct this afterward if you don't get to it before me.

comment:4 Changed 3 years ago by atagar

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from needs_revision to closed

Hi Nick, the testing failures were due to the third patch. I can certainly see your reason for it - when I saw list had a pop first thought was constant time for both ends but on reflection that would only work if there was an offset counter for the array head (which on reflection was foolish to expect, and looks to not be the case).

I wasn't clear from the description how much it bought us in practice so I went ahead and pushed the other two. Patches welcome for correcting the pop() if it passes the tests.

comment:5 Changed 3 years ago by atagar

Forgot to mention - thanks for the patches! I greatly appreciate these sort of performance improvements. :)

Note: See TracTickets for help on using tickets.