Opened 8 years ago

Closed 8 years ago

#4283 enhancement closed fixed (fixed)

Improve lookupRecordType in names

Reported by: fijal Owned by:
Priority: normal Milestone:
Component: names Keywords: pypy
Cc: jesstess Branch: branches/unglobal-lookuprecordtype-4283
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

lookupRecordType in names is using globals() right now and a lookup of a constructed string. Would be much better (for pypy especially) to have a dictionary name -> record type instead.

Change History (11)

comment:1 Changed 8 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/unglobal-lookuprecordtype-4283

(In [28549]) Branching to 'unglobal-lookuprecordtype-4283'

comment:2 Changed 8 years ago by Jean-Paul Calderone

(In [28550]) Use a specially constructed lookup dict instead of globals()

refs #4283

comment:3 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Suggestion implemented in the branch. I think the existing tests cover this code pretty well; the ones handling actually dns requests all need to call it, anyway. The only tests I could think to add would be ones that call lookupRecordType directly and assert that the returned value is exactly the expected class object. There'd need to be about 30 of these, and they amount to just a duplication of the information present in the Record_* class names.

comment:4 Changed 8 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: set to Jean-Paul Calderone

Thanks for the branch, exarkun. The implementation looks strictly less scary than what's in trunk. A few comments:

  • There's some trailing whitespace in the file
  • In lookupRecordType: @param and @return need @types
  • class Message needs a docstring.
  • There aren't any tests along the code path where lookupRecordType returns None (although I admit it doesn't look like a very interesting case because parseRecords gets a num of 0 several times in the tests already).
  • I had to poke around and prove to myself that the del name line made sense. Maybe warrants a comment?
  • builds are happy.

comment:5 Changed 8 years ago by Jean-Paul Calderone

(In [28578]) Address review comments

  1. Clean up trailing whitespace in dns.py
  2. Add more epytext markup to lookupRecordType
  3. Add a class docstring for Message
  4. Add a test for lookupRecordType returning None
  5. Add a comment about the del name in the Message class scope

refs #4283

comment:6 Changed 8 years ago by Jean-Paul Calderone

(In [28579]) Use the right name

refs #4283

comment:7 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Thanks for the awesome review! All points addressed, I think.

comment:8 Changed 8 years ago by jesstess

Keywords: review removed
Owner: set to Jean-Paul Calderone
  • test_dns.py needs a copyright bump
  • class MessageTestCase needs a docstring

Other than that, looks good to merge. Thanks!

comment:9 Changed 8 years ago by Jean-Paul Calderone

(In [28588]) Address review feedback

refs #4283

comment:10 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [28590]) Merge unglobal-lookuprecordtype-4283

Author: exarkun Reviewer: jesstess Fixes: #4283

Stop using globals() in Twisted Names to look up record classes by type. Instead, construct a dictionary just holding this information which can be keyed without constructing specially formatted strings. This has nice performance consequences on PyPy.

comment:11 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.