[PATCH] smartset: add a "toset" method

[Martin von Zweigbergk]

On Jun 3, 2017 3:51 PM, "Augie Fackler" <raf at durin42.com> wrote:


> On Jun 3, 2017, at 6:48 PM, Martin von Zweigbergk <martinvonz at google.com>
wrote:
>
>
>
> On Jun 3, 2017 3:36 PM, "Augie Fackler" <raf at durin42.com> wrote:
>
>> On Jun 3, 2017, at 4:58 PM, Jun Wu <quark at fb.com> wrote:
>>
>> Excerpts from Jun Wu's message of 2017-06-03 13:54:18 -0700:
>>> Excerpts from Augie Fackler's message of 2017-06-03 16:25:52 -0400:
>>>> I like where you're headed with this,
> Where is Jun headed? It'd be nice to see the user of this new method and
see the it helps there (in practice). The change is simple and makes sense,
but it's still dead code at this point.

There are a reasonable number of places where we turn a smartset into a
regular set, and right now there’s a fair amount of bonus overhead for some
of the smartset instances because they already have a set that could be
quickly copied by cpython all in native code, but instead we add a layer of
iterator indirection which requires a lot of bouncing between native code
and bytecode. For cases that can efficiently just dupe an existing set and
immediately return it, this will be a nice little win.


And I suppose in at least one of those, the smartset is a baseset? Sounds
good to me then.


>>>> but I think I'd rather have
>>>> classes with an efficient way to produce a set implement toset
>>>> themselves and leave the default implementation as set(self) so we can
>>>> avoid the spooky action at a distance implied by this hasattr().
>>>
>>> The problem is the lack of ability to customize "set(smartset)" logic
in the
>>> Python language. "set" will always treat "smartset" as an iterable,
which
>>> makes the operation O(N log N) even if we have O(1) fast path.
>>>
>>> Between syntactic simplicity and ability to have fast code path, I want
to
>>> choose the latter.
>>>
>>> For "hasattr", do you mean we might need to do hasattr(x, 'toset')? That
>>> looks like a sign of poorly designed APIs (sometimes return a smartset,
>>> sometimes return a set, so you never know what "x" is). It shouldn't
happen
>>> in a clean codebase.
>>
>> Sorry. I misunderstood the whole paragraph. I'll move "toset" from
>> "abstractsmartset" to subclasses.
>
> You should probably still have a toset() on abstractsmartset if you
intend for it to be a mandatory part of the interface (which I recommend,
per Liskov), but have it either do set(self) or (probably better) raise
NotImplementedError.
>
> Unless I'm reading it wrong, he did add that.

I meant to keep it in a v2, without the hasattr() abstraction layer
violation. :)


Maybe it's just poor diff context, but it looks like the function is on
abstractsmartset in v2 and without the hasattr hack.


>
>
>>
>> Good advice by the way!
>
> Thanks! Glad you like it. :)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170603/200917a3/attachment.html>