https://pantsbuild.org/ logo
Join Slack
Powered by
so, it looks like <https://github.com/pantsbuild/p...
# development
w
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
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
Revert sounds fine for me in 1.26.x. For 1.27.x, we can redesign the implementation while keeping the same API
w
going to spend a bit more time on it today... if i run into a wall will go for the revert.
👍 1
h
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
so... i'm not sure why they are.
...not hashable.
h
Dicts are mutable so not safe to hash
w
...oh. actually, that's clear. maybe. but not for the set.
ehh, if you don't expose any of the methods.
h
Maybe you could use frozenset + tuple? Frozenset gives cheap lookups, tuple maintains order
w
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
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
yep
💯 1
h
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
yea
Open in Slack
PreviousNext
Powered by