{"action":"created","issue":{"url":"https://api.github.com/repos/rdkit/rdkit/issues/929","repository_url":"https://api.github.com/repos/rdkit/rdkit","labels_url":"https://api.github.com/repos/rdkit/rdkit/issues/929/labels{/name}","comments_url":"https://api.github.com/repos/rdkit/rdkit/issues/929/comments","events_url":"https://api.github.com/repos/rdkit/rdkit/issues/929/events","html_url":"https://github.com/rdkit/rdkit/pull/929","id":157704730,"number":929,"title":"Dev/rdkit rdany int keys","user":{"login":"bp-kelley","id":9888416,"avatar_url":"https://avatars.githubusercontent.com/u/9888416?v=3","gravatar_id":"","url":"https://api.github.com/users/bp-kelley","html_url":"https://github.com/bp-kelley","followers_url":"https://api.github.com/users/bp-kelley/followers","following_url":"https://api.github.com/users/bp-kelley/following{/other_user}","gists_url":"https://api.github.com/users/bp-kelley/gists{/gist_id}","starred_url":"https://api.github.com/users/bp-kelley/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bp-kelley/subscriptions","organizations_url":"https://api.github.com/users/bp-kelley/orgs","repos_url":"https://api.github.com/users/bp-kelley/repos","events_url":"https://api.github.com/users/bp-kelley/events{/privacy}","received_events_url":"https://api.github.com/users/bp-kelley/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2016-05-31T15:45:17Z","updated_at":"2016-06-01T17:17:07Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/rdkit/rdkit/pulls/929","html_url":"https://github.com/rdkit/rdkit/pull/929","diff_url":"https://github.com/rdkit/rdkit/pull/929.diff","patch_url":"https://github.com/rdkit/rdkit/pull/929.patch"},"body":"Major optimization to RDKit Dictionaries.\r\n\r\n* Dictionaries no longer use string->value lookups, they use int->value lookups.\r\n\r\n* Most internal operations have predefined names (now integer values) so these are largely unchanged in the source code when using the common_properties namespace.\r\n\r\n* names are mapped to integers using a new tagging system;  \r\n\r\n```\r\n  common_properties::getPropName(int) //returns the name for an integer tag\r\n  Dict::tagmap.get(\"name\") //returns the tag for the string, making a new tag if necessary in a thread-safe manner\r\n```\r\n\r\n* the list of computedProperties no longer exists in the dictionary as it was always used.  It now exists outside of the dictionary and is a list of integers.  This adds Dict::isComputedProp(what) and Dict::getComputedPropList() which are faster to compute and search as well.\r\n\r\nBenefits:\r\n  Smaller and quite a bit faster than the original implementation, especially for \"built in\" types.\r\n\r\nRisks:\r\n  The default implementation can only handle 16,777,216 (2^24) tags.  An exception will occur when this number is reached with essentially no recurse to recover tags.  This may be an issue for long running servers.  At the cost of some space per value (32bits) or by using the double-magic implementation, this can be 2^32 or 4,294,967,296.  \r\n\r\nSee the RDVALUE_HAS_KEY define in RDValue.h.\r\n\r\nBoth have the same issue that the tag space cannot be recovered unless we know that no molecules are present.\r\n"},"comment":{"url":"https://api.github.com/repos/rdkit/rdkit/issues/comments/223062517","html_url":"https://github.com/rdkit/rdkit/pull/929#issuecomment-223062517","issue_url":"https://api.github.com/repos/rdkit/rdkit/issues/929","id":223062517,"user":{"login":"bp-kelley","id":9888416,"avatar_url":"https://avatars.githubusercontent.com/u/9888416?v=3","gravatar_id":"","url":"https://api.github.com/users/bp-kelley","html_url":"https://github.com/bp-kelley","followers_url":"https://api.github.com/users/bp-kelley/followers","following_url":"https://api.github.com/users/bp-kelley/following{/other_user}","gists_url":"https://api.github.com/users/bp-kelley/gists{/gist_id}","starred_url":"https://api.github.com/users/bp-kelley/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bp-kelley/subscriptions","organizations_url":"https://api.github.com/users/bp-kelley/orgs","repos_url":"https://api.github.com/users/bp-kelley/repos","events_url":"https://api.github.com/users/bp-kelley/events{/privacy}","received_events_url":"https://api.github.com/users/bp-kelley/received_events","type":"User","site_admin":false},"created_at":"2016-06-01T17:17:07Z","updated_at":"2016-06-01T17:17:07Z","body":"@greglandrum I think it is pretty large.  The tests themselves run 8-15% faster as a whole from the already-improved Dict lookups, which is pretty impressive I think.\r\n\r\nYou may note that the master branch has already become a vector lookup: \r\n\r\nhttps://github.com/rdkit/rdkit/blob/master/Code/RDGeneral/Dict.h\r\n\r\nBut this is hampered by the non-cache locality of the string storage.\r\n\r\nI did a smiles reading benchmark back in the day that I can resurrect with the new code base (and proposed fixes above).  And yes, the thread-safety for the tagging system is a concern, but the cache-locality of searching the vectors more than makes up for it, I think.  Still, the more complicated the plumbing...\r\n\r\nI'll do some benchmarks this weekend and valgrind runs to see if I can justify the complexity.  This was more a matter of \"Can I make this work?\" than trying to shove it in where it doesn't fit.\r\n\r\n"}}
{"actor":{"display_login":"bp-kelley"}}