Opened 5 years ago

Closed 5 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
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

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 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/unglobal-lookuprecordtype-4283

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

comment:2 Changed 5 years ago by exarkun

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

refs #4283

comment:3 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 5 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner set to exarkun

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 5 years ago by exarkun

(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 5 years ago by exarkun

(In [28579]) Use the right name

refs #4283

comment:7 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

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

comment:8 Changed 5 years ago by jesstess

  • Keywords review removed
  • Owner set to exarkun
  • test_dns.py needs a copyright bump
  • class MessageTestCase needs a docstring

Other than that, looks good to merge. Thanks!

comment:9 Changed 5 years ago by exarkun

(In [28588]) Address review feedback

refs #4283

comment:10 Changed 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.