https://pantsbuild.org/ logo
w

witty-crayon-22786

03/14/2020, 11:59 PM
so, it looks like https://github.com/pantsbuild/pants/pull/9181 and https://github.com/pantsbuild/pants/pull/9166 are a significant regression: around 3x for
./pants filter ::
in pantsbuild pants, and a lot more internally.
x.svg
the
__contains__
bottleneck was called out in https://github.com/pantsbuild/pants/pull/9166, but i think it was dismissed with 1) too small a set, 2) items with an eq impl that was too cheap
i'll look at this for a bit today, but it looks like it would also be a relatively easy thing to revert out of 1.26.x and follow up on.
👍 1
h

hundreds-father-404

03/15/2020, 12:23 AM
Revert sounds fine for me in 1.26.x. For 1.27.x, we can redesign the implementation while keeping the same API
w

witty-crayon-22786

03/15/2020, 12:24 AM
going to spend a bit more time on it today... if i run into a wall will go for the revert.
👍 1
h

hundreds-father-404

03/15/2020, 12:26 AM
Check out the Ordered Set library. I think the best approach is to go back to having a list and a dict. But, I’m not sure how that would work with FOS because dicts are mutable :/
We use contains a lot in the target API, so I think getting that back down to O(1) is worth the extra space complexity
w

witty-crayon-22786

03/15/2020, 12:26 AM
so... i'm not sure why they are.
...not hashable.
h

hundreds-father-404

03/15/2020, 12:27 AM
Dicts are mutable so not safe to hash
w

witty-crayon-22786

03/15/2020, 12:27 AM
...oh. actually, that's clear. maybe. but not for the set.
ehh, if you don't expose any of the methods.
h

hundreds-father-404

03/15/2020, 12:28 AM
Maybe you could use frozenset + tuple? Frozenset gives cheap lookups, tuple maintains order
w

witty-crayon-22786

03/15/2020, 12:28 AM
giving it a swing with just the dict. because "hash" is "iterate through all keys and hash them".
👍 1
and the frozenness is just a matter of whether you expose those methods.
h

hundreds-father-404

03/15/2020, 12:33 AM
Oh is the idea to solely use dict? That’s a great idea. Even if you have to override hash, that would be perfect. And you don’t need any complementary data type because dicts are ordered in 3.6+
w

witty-crayon-22786

03/15/2020, 12:33 AM
yep
💯 1
h

hundreds-father-404

03/15/2020, 12:34 AM
Awesome. Then you could use an identical implementation between OS and FOS. But, we only want to implement hash for FOS. It should eagerly fail when trying to use OS with the engine because it’s not hahable
w

witty-crayon-22786

03/15/2020, 12:34 AM
yea