{"action":"created","issue":{"url":"https://api.github.com/repos/scrapy/scrapy/issues/3420","repository_url":"https://api.github.com/repos/scrapy/scrapy","labels_url":"https://api.github.com/repos/scrapy/scrapy/issues/3420/labels{/name}","comments_url":"https://api.github.com/repos/scrapy/scrapy/issues/3420/comments","events_url":"https://api.github.com/repos/scrapy/scrapy/issues/3420/events","html_url":"https://github.com/scrapy/scrapy/pull/3420","id":360001233,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE1MzY0NjIy","number":3420,"title":"request_fingerprint should return bytes","user":{"login":"nyov","id":438293,"node_id":"MDQ6VXNlcjQzODI5Mw==","avatar_url":"https://avatars0.githubusercontent.com/u/438293?v=4","gravatar_id":"","url":"https://api.github.com/users/nyov","html_url":"https://github.com/nyov","followers_url":"https://api.github.com/users/nyov/followers","following_url":"https://api.github.com/users/nyov/following{/other_user}","gists_url":"https://api.github.com/users/nyov/gists{/gist_id}","starred_url":"https://api.github.com/users/nyov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nyov/subscriptions","organizations_url":"https://api.github.com/users/nyov/orgs","repos_url":"https://api.github.com/users/nyov/repos","events_url":"https://api.github.com/users/nyov/events{/privacy}","received_events_url":"https://api.github.com/users/nyov/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-09-13T17:56:12Z","updated_at":"2018-09-14T23:43:48Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/scrapy/scrapy/pulls/3420","html_url":"https://github.com/scrapy/scrapy/pull/3420","diff_url":"https://github.com/scrapy/scrapy/pull/3420.diff","patch_url":"https://github.com/scrapy/scrapy/pull/3420.patch"},"body":"In my opinion `request_fingerprint` (`scrapy.utils.request`) would be more sensible to return `bytes`; as returned by `hashlib.sha1().digest()` (versus `.hexdigest()`).\r\n\r\nA hash as returned by this function is usually used for comparison only (duplicate detection) and being kept in memory (rfp cache) -- except for the cache middleware where it's used as a unique storage key.\r\n\r\nSince this is happening mostly under the hood, comparing between bytes-objects is just as good; but storing the 20 bytes SHA1, over a 40 byte hexadecimal string-representation, is a theoretical 200% improvement in memory usage.\r\n(In reality `sys.getsizeof()` makes the `bytes` object *53* bytes worth and the `str` (unicode) *89*, not quite double the size.)\r\n\r\nThe only place where I can see a hex representation used, is as a unique filename for the `FilesystemCacheStorage`; even the DB storages could save some by storing the plain binary representation; they do `to_bytes()` the hexstring instead. This is of course not the inverse of turning bytes into a hex string (that would be `bytes(bytearray.fromhex())`).\r\n\r\nAlas, this is a behavior breaking change which could impact 3rd party code, so I don't have much hope of seeing the current functionality changing. But perhaps it could still return bytes when passing a flag - and be used with that internally. It's much easier to ask for that, than turning the hex string back to \"binary\"."},"comment":{"url":"https://api.github.com/repos/scrapy/scrapy/issues/comments/421511508","html_url":"https://github.com/scrapy/scrapy/pull/3420#issuecomment-421511508","issue_url":"https://api.github.com/repos/scrapy/scrapy/issues/3420","id":421511508,"node_id":"MDEyOklzc3VlQ29tbWVudDQyMTUxMTUwOA==","user":{"login":"nyov","id":438293,"node_id":"MDQ6VXNlcjQzODI5Mw==","avatar_url":"https://avatars0.githubusercontent.com/u/438293?v=4","gravatar_id":"","url":"https://api.github.com/users/nyov","html_url":"https://github.com/nyov","followers_url":"https://api.github.com/users/nyov/followers","following_url":"https://api.github.com/users/nyov/following{/other_user}","gists_url":"https://api.github.com/users/nyov/gists{/gist_id}","starred_url":"https://api.github.com/users/nyov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nyov/subscriptions","organizations_url":"https://api.github.com/users/nyov/orgs","repos_url":"https://api.github.com/users/nyov/repos","events_url":"https://api.github.com/users/nyov/events{/privacy}","received_events_url":"https://api.github.com/users/nyov/received_events","type":"User","site_admin":false},"created_at":"2018-09-14T23:43:48Z","updated_at":"2018-09-14T23:43:48Z","author_association":"CONTRIBUTOR","body":"Thanks for looking at it, @kmike.\r\nThis has been a quick-fix and more to see if I could actually find all dependencies on `request_fingerprint`. It's not my best effort as I didn't have much hope for it anyway.\r\n\r\nI haven't done a profiling run in practice so it may be these savings for some reason don't aggregate in real-world usage - though I'd find that strange.\r\nHowever I know that some dbs can make use of an optimized storage type for these size-objects - postgres for example has an UUID type that is more efficient than storing the same length as bytes.\r\n\r\nThe requirement for `binascii` is just ugly, but for the `dupefilter`'s disk-cache to remain backwards-compatible in format it would need to continue saving strings - besides I couldn't find a sane way to split bytes on \"newlines\". I can't split them up on ASCII-0, as the SHA1s can have those - so some magic marker would be needed. Sticking with strings and newlines was just easiest for now.\r\n\r\nFurther, I'd rather be rid of the `as_string` argument, always only return bytes, and have everyone else do `rfp.hex()` where a string is needed -- if the PY2-way to do that wasn't so verbose. I know changing return type of a function with a flag is bad design, but the convenience (for PY2) here is just so strong I'm undecided which is better.\r\n(Perhaps you'd know a way to wrap the return in a custom container subclass which will also understand `.hex()` in PY2, without that nixing the memory benefit of changing to bytes in the first place?)"}}
{"actor":{"display_login":"nyov"}}