{"action":"created","issue":{"url":"https://api.github.com/repos/RIPE-NCC/ripe.atlas.sagan/issues/73","repository_url":"https://api.github.com/repos/RIPE-NCC/ripe.atlas.sagan","labels_url":"https://api.github.com/repos/RIPE-NCC/ripe.atlas.sagan/issues/73/labels{/name}","comments_url":"https://api.github.com/repos/RIPE-NCC/ripe.atlas.sagan/issues/73/comments","events_url":"https://api.github.com/repos/RIPE-NCC/ripe.atlas.sagan/issues/73/events","html_url":"https://github.com/RIPE-NCC/ripe.atlas.sagan/pull/73","id":132943584,"number":73,"title":"Do the bulk of the validity checking for ParsingDict.keys() at class …","user":{"login":"chrisamin","id":189090,"avatar_url":"https://avatars.githubusercontent.com/u/189090?v=3","gravatar_id":"","url":"https://api.github.com/users/chrisamin","html_url":"https://github.com/chrisamin","followers_url":"https://api.github.com/users/chrisamin/followers","following_url":"https://api.github.com/users/chrisamin/following{/other_user}","gists_url":"https://api.github.com/users/chrisamin/gists{/gist_id}","starred_url":"https://api.github.com/users/chrisamin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chrisamin/subscriptions","organizations_url":"https://api.github.com/users/chrisamin/orgs","repos_url":"https://api.github.com/users/chrisamin/repos","events_url":"https://api.github.com/users/chrisamin/events{/privacy}","received_events_url":"https://api.github.com/users/chrisamin/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2016-02-11T11:03:32Z","updated_at":"2016-02-12T09:53:50Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RIPE-NCC/ripe.atlas.sagan/pulls/73","html_url":"https://github.com/RIPE-NCC/ripe.atlas.sagan/pull/73","diff_url":"https://github.com/RIPE-NCC/ripe.atlas.sagan/pull/73.diff","patch_url":"https://github.com/RIPE-NCC/ripe.atlas.sagan/pull/73.patch"},"body":"Currently ParsingDict.keys() checks every single attribute in dir(obj) to see if it should return it. This includes actually getattr()ing every attribute and checking whether it is callable(). Since Sagan objects do not have functions as attributes except when they are defined on the class, we can do this check once at class instantiation time. We still have to filter out underscore-private attributes in keys(), but that's nowhere near as expensive.\r\n\r\nMy use case for this change is the timetravel API as called by the map on the RIPE Atlas measurement detail page, where for a builtin traceroute measurement (with three layers of nested ParsingDicts: Traceroute -> Hop -> Packet) it can cut API response time by up to a factor of 3."},"comment":{"url":"https://api.github.com/repos/RIPE-NCC/ripe.atlas.sagan/issues/comments/183256792","html_url":"https://github.com/RIPE-NCC/ripe.atlas.sagan/pull/73#issuecomment-183256792","issue_url":"https://api.github.com/repos/RIPE-NCC/ripe.atlas.sagan/issues/73","id":183256792,"user":{"login":"chrisamin","id":189090,"avatar_url":"https://avatars.githubusercontent.com/u/189090?v=3","gravatar_id":"","url":"https://api.github.com/users/chrisamin","html_url":"https://github.com/chrisamin","followers_url":"https://api.github.com/users/chrisamin/followers","following_url":"https://api.github.com/users/chrisamin/following{/other_user}","gists_url":"https://api.github.com/users/chrisamin/gists{/gist_id}","starred_url":"https://api.github.com/users/chrisamin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chrisamin/subscriptions","organizations_url":"https://api.github.com/users/chrisamin/orgs","repos_url":"https://api.github.com/users/chrisamin/repos","events_url":"https://api.github.com/users/chrisamin/events{/privacy}","received_events_url":"https://api.github.com/users/chrisamin/received_events","type":"User","site_admin":false},"created_at":"2016-02-12T09:53:50Z","updated_at":"2016-02-12T09:53:50Z","body":"I did a bit more digging and it seems that the call to keys() comes from __len__(), which in turn comes from this line in ripe.atlas.sagan.traceroute:\r\n\r\nrtts = sorted([p.rtt for p in self.packets if p and p.rtt])\r\n\r\nwhere \"p\" is a Packet (ParsingDict). So evaluating a parsing dict as a boolean ultimately key() to see whether it's \"empty\" or not. This is standard Python truth-testing behaviour:\r\n\r\n\" * instances of user-defined classes, if the class defines a __nonzero__() or __len__() method, when that method returns the integer zero or bool value False.2.5 \"\r\n\r\nI can fix this by not testing for the truthiness of Packets, since keys() can never return an empty list anyway: the fields are there by default, they're just None. We can also hardcode __nonzero__() so that a ParsingDict is always True (or raises an exception because it's meaningless), just in case somebody stumbles upon this in the future.\r\n\r\nAll that being said, I still don't like that every call to keys() involves getattr()ing every single attribute *and* calling callable() on it, when we know before the object is even initialized what its methods are going to be. What if I implement similar functionality using a class decorator? Something like this:\r\n\r\ndef add_method_names(cls):\r\n   cls.method_names = set()\r\n   for key in dir(cls):\r\n     if getattr(cls, key) is a method:\r\n       cls.method_names.add(key)\r\n\r\n@add_method_names\r\nclass ParsingDict(object):\r\n   def keys(self):\r\n     return [all the keys if they're not in self.method_names]"}}