Walter Bright wrote:
> Sean Kelly wrote:
>> Walter Bright wrote:
>>> http://www.digitalmars.com/d/std_signals.html
>>>
>>> And that, hopefully, is that.
>>
>> Nice work!  In skimming the code, one thing jumped out at me:
>>
>> signals.d:171: len += len + 4;
>>
>> I think this should be: len += 4;
>>
>> Or actually: len += (void*).sizeof;
> 
> No, that is as intended. It's using a power of 2 allocation method. Also, the sizeof thing is taken care of by the call to calloc/realloc.

Then it's massively over-allocating. Note that it performs a call to calloc/realloc *every single time* the function is called. It (more than) doubles the length and sets the last element to the new slots.

Walter Bright wrote:
> Sean Kelly wrote:
>> Walter Bright wrote:
>>> http://www.digitalmars.com/d/std_signals.html
>>>
>>> And that, hopefully, is that.
>>
>> Nice work!  In skimming the code, one thing jumped out at me:
>>
>> signals.d:171: len += len + 4;
>>
>> I think this should be: len += 4;
>>
>> Or actually: len += (void*).sizeof;
> 
> No, that is as intended. It's using a power of 2 allocation method.

Okay, now I'm confused.  Here's the pertinent bit of code, edited for clarity:

    auto len = slots.length;
    auto startlen = len;

    len += len + 4;
    auto p = std.c.stdlib.realloc(slots.ptr, slot_t.sizeof * len);
    slots = (cast(slot_t*)p)[0 .. len];
    slots[startlen] = slot;

This looks like the allocated buffer will never be more than half full, because a reallocation of size len * 2 occurs every time connect is called.  This should give buffer sizes of: 4, 8, 20, 44, 92, etc, with the new element added at 0, 4, 8, 20, and 44, respectively.  In fact, it seems the buffer density will decrease over time because the space between elements doubles every insertion?  Hrm, perhaps this line:

    slots = (cast(slot_t*)p)[0 .. len];

should be more like this:

    slots = (cast(slot_t*)p)[0 .. startlen + 1];

so the next call to slots.length isn't aware of the unused space?

>> To be 64-bit safe.  Finally, is there any reason to use _d_toObject in this code instead of cast(Object)?  I'd think they would do the same thing?
> 
> Not exactly. cast(Object)p won't do the pointer adjustment if the type of p is a void*. To convert an interface to its underlying Object, the cast implementation needs to know what the interface type is.

Oh I see, so this is because p can be an interface as well.  Thanks.


Sean

Walter Bright wrote:
> The fix is straightforward, in std\signals.d just make the imports public:
> 
>> public import std.stdio;
>> public import std.c.stdlib;
>> public import std.outofmemory;

Maybe instead, it should be something like this?
# public static import signals_stdio = std.stdio;
# public static import signals_c_stdlib = std.c.stdlib;
# public static import signals_outofmemory = std.outofmemory;

These are modules that user modules are plenty likely not to want in their namespace, so it would be good to put a little indirection like this in place.  Just two cents.

-- Chris Nicholson-Sauls

Sean Kelly wrote:
> Hrm, perhaps this line:
> 
>     slots = (cast(slot_t*)p)[0 .. len];
> 
> should be more like this:
> 
>     slots = (cast(slot_t*)p)[0 .. startlen + 1];
> 
> so the next call to slots.length isn't aware of the unused space?

On second thought, scratch this idea.  It would cause linear growth and half the buffer would still go unused.


Sean

Frits van Bommel wrote:
> Walter Bright wrote:
>> No, that is as intended. It's using a power of 2 allocation method. Also, the sizeof thing is taken care of by the call to calloc/realloc.
> 
> Then it's massively over-allocating. Note that it performs a call to calloc/realloc *every single time* the function is called. It (more than) doubles the length and sets the last element to the new slots.

Looks like I screwed that up!

"Walter Bright" <newshound@digitalmars.com> wrote in message news:eidh0g$10e1$1@digitaldaemon.com...

>> public import std.stdio;
>> public import std.c.stdlib;
>> public import std.outofmemory;

That then works when the class has one signal, but trying to put more than one:

class Input
{
    mixin Signal!(int, int) click;
    mixin Signal!(char) keyDown;
}

Gives..

C:\dmd\bin\..\src\phobos\std\signals.d(181): Error:
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(196): Error:
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(232): Error:
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(181): Error:
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(196): Error:
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(232): Error:
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with
dtest.Input.Signal!(char).unhook at dtest.d(206)
> Execution finished.

:S

Mixins, mixins, mixins.

Walter Bright wrote:
> Chris Miller wrote:
>> I think this is finally a real mixin limitation being exposed. You probably only tested std.signals inside the signals.d source file, where the mixin had access to std.signals' imports.
> 
> I plead guilty. My test suite is now corrected.
> 
>> But use std.signals from another file and the mixin cannot access std.signal's imports because it's accessing the mixed-in scope. In other words, I think to remove these errors, std.signals' imports would need to be imported inside the mixin template (hack?), or change how mixins work.
> 
> The fix is straightforward, in std\signals.d just make the imports public:
> 
>> public import std.stdio;
>> public import std.c.stdlib;
>> public import std.outofmemory;

Walter, please don't pull a GW here.

On Thu, 02 Nov 2006 14:30:25 -0500, Walter Bright <newshound@digitalmars.com> wrote:

> Chris Miller wrote:
>> I think this is finally a real mixin limitation being exposed. You probably only tested std.signals inside the signals.d source file, where the mixin had access to std.signals' imports.
>
> I plead guilty. My test suite is now corrected.
>
>> But use std.signals from another file and the mixin cannot access std.signal's imports because it's accessing the mixed-in scope. In other words, I think to remove these errors, std.signals' imports would need to be imported inside the mixin template (hack?), or change how mixins work.
>
> The fix is straightforward, in std\signals.d just make the imports public:
>
>> public import std.stdio;
>> public import std.c.stdlib;
>> public import std.outofmemory;

Yuck. Did somebody say unprovoked import conflict? How about making all imports public! HOORAY

Walter Bright wrote:
> The fix is straightforward, in std\signals.d just make the imports public:
> 
>> public import std.stdio;
>> public import std.c.stdlib;
>> public import std.outofmemory;

Sorry Walter, but the name for it is 'hack', not 'fix' :( How about fixing mixins ?

Walter Bright wrote:
> http://www.digitalmars.com/d/std_signals.html
> 
> And that, hopefully, is that.

Obviously not...

Here's one incorporating the suggested improvements:

http://www.digitalmars.com/d/phobos/signal.d