dlang
A closure with a layout of pointer size or below that is not modified, should not have a closure
@TurkeyMan wrote:
error : function `MyClass.myMethod` is `@nogc` yet allocates closure for `myMethod()` with the GC
delegate `MyClass.myMethod.__lambda3` closes over variable `this`
class MyClass
{
void doSomething();
void myMethod() @nogc
{
acceptsCallback(¬Lambda); // this works; delegate is created from the method
acceptsCallback((){ doSomething(); }); // doesn't work! closure needlessly tries to allocate
}
void notLambda()
{
doSomething();
}
}
I think this is essentially a bug and a serious usability hindrance.
The lambda seems to try and allocate a closure because it captures this, but that's no reason to allocate a closure!
A function that receives this as context is called a method... in the extremely common case where a lambda closes this and nothing else, the lambda should just be synthesised as a method of this rather than a closure. The double-indirection from the closure is pointless and inefficient anyway.
This would make lambda's about 100x more useful in @nogc code, because they can naturally access this as context without any closure allocation.
alphaglosined commented on 2024-10-30T15:20:35Z
This is a misunderstanding of what ``@nogc`` is.
It is a linting tool. It guarantees that no GC allocation will take place within the function.
The reason why the compiler is not putting the closure on the stack is because it could escape the call stack. Mark the delegate parameter in ``acceptsCallback`` as ``scope`` and this will work.
Without the escaping guarantees, using the GC is the correct solution in this situation, and therefore should error.
turkeyman commented on 2024-10-30T15:28:30Z
Please think that through.
tim.dlang commented on 2024-10-30T16:27:30Z
This is independent of @nogc. Code not marked @nogc would also benefit from this.
turkeyman commented on 2024-10-30T16:30:36Z
Correct, all code will benefit.
I just made the point because it's just particularly important for @nogc code, where it is currently impossible to implement event-based systems conveniently.
dfj1esp02 commented on 2024-10-30T21:48:27Z
Lowering could be something like
---
struct Context
{
T* ref_capture; //normal capture
}
void Closure(Context* ctx)
{
bool oneref=false; //change this
T* local_ref;
if(oneref)local_ref=cast(T*)ctx;
else local_ref=ctx.ref_capture;
//call
local_ref.method();
}
---
If one reference is captured, `oneref` variable could be changed(?) accordingly, and argument would be different, then backend will optimize it.
timon.gehr commented on 2024-10-30T23:27:25Z
This is a valid enhancement request.
bugzilla (@WalterBright) commented on 2024-11-10T21:34:15Z
Let's rewrite:
class MyClass
{
void doSomething();
void myMethod() @nogc
{
acceptsCallback((){ doSomething(); });
}
}
to make visible what is happening.
class MyClass
{
void doSomething(MyClass this);
void myMethod(MyClass this) @nogc
{
auto outer = &this;
void func(MyClass* outer)
{
doSomething(*outer);
}
acceptsCallback(&outer.func);
}
}
func is a member of myMethod, not a member of MyClass. func's "this" pointer is a reference to the stack frame of myMethod, not a reference to MyClass. acceptsCallback() is allowed to squirrel away the address of func(), and could access it after myMethod() has exited. This will result in corrupted memory.
The solution is for acceptsCallback to annotate its parameter with `scope`, which says it will not preserve its argument past the lifetime of acceptsCallback.
turkeyman commented on 2024-11-11T01:55:16Z
I think you've completely missed the point...
Yes, me code DOES squirrel away the address of func, that is literally the point of this bug report.
The point is, the lambda doesn't access anything at all from the closure except 'this', and so rather than creating a closure like:
struct Closure
{
MyClass this;
}
void func(Closure* closure) { closure.this.doSomething(); }
The closure is pointlessly allocated; there's no reason to synthesise the function this way. If the Closure ONLY references `MyClass this`, then func has no reason to allocate a closure and bind it to the delegate; it could just synthesise this function:
func(MyClass this) { this.doSomething(); }
This bypasses the closure since it has no reason to exist, and just places `this` into the delegate, and so for all intents and purposes, the lambda is just a normal method.
This optimisation is similarly valid for any closure which closes over exactly one pointer. It could even further be generalised such that any closure which closes over exactly a single size_t-sized POD just write that value to the 'this' pointer of the delegate (essentially small-struct optimisation embedded in the delegate), and then the lambda naturally receives it as first argument when the delegate it called.
There's a quite broad category of optimisation here; it's inefficient and pointless in most cases for a closure to be allocated with just one member... and it also has the nice advantage that in cases where a delegate has only a single reference (it, 'this'), then it doesn't need to allocate a closure at all, and so it's naturally `@nogc`.
alphaglosined commented on 2024-11-11T02:35:00Z
Okay now it starts to make sense what you are wanting.
If the closure's layout is a pointer in size or less you do not want it to exist.
It does require that this value is not modified, although what a pointer points to may be modified (head const).
That leads me to the question for Walter, how much in the frontend needs to change for this optimization to be implemented by ldc/gdc?
If it does not need to be changed, then this should be viewed as a ldc/gdc bug not a dmd one (which you shouldn't be using when optimizations are needed).
turkeyman commented on 2024-11-11T02:38:17Z
I promise you it made sense from the start ;)
turkeyman commented on 2024-11-11T03:25:05Z
This is a language thing; GDC/LDC have nothing to say about this.
The frontend implements this, it's semantic to detect if a closure only contains a single thing and then delete it.
Yes, you touched on one issue with mutability of references to stack objects; however `this` is not mutable, it's impossible for `this` to change during the lifetime of the closure, which is why my initial request applies to `this`... any expansion of this optimisation is theoretical, but the case for `this`, which is overwhelmingly the most common and the most useful case should be addressed initially.
Also, why did you rename my issue? :/
alphaglosined commented on 2024-11-11T03:33:47Z
I renamed it to translate it into compiler speak.
The ``@nogc`` aspect has nothing to do with the enhancement.
As for ldc/gdc, yes it is a them problem.
Closure creation is part of the glue code, not the frontend itself.
https://github.com/dlang/dmd/blob/b70e66033c7f53b3ac7def31e49d803ff87d6d30/compiler/src/dmd/toir.d#L779
turkeyman commented on 2024-11-11T07:29:30Z
There is no closure creation when this case is detected.
@nogc does have something to do with the enhancement, because the enhancement will make code that doesn't currently work in @nogc work after the fix... and that's specifically why I need this fixed.
zero commented on 2024-11-14T20:34:09Z
Created attachment 1923
Very crude patch, just to pin point the relevant code path.
I made a whack a mole (literally) attempt on this issue.
The code works on the provided code and closure tests in dmd "test suite" (to best of my knowledge how to run them).
I think it is very fragile, and never got to do much more with it, but better to let it rot here than on my drive.
This keeps coming up for me again and again and again.
Anyone who uses @nogc and also tries to use lambdas will very quickly encounter this issue.
A delegate has one pointer of context; and while it's often the this pointer which may point to a closure object, it doesn't need to be a pointer at all; if the closure is <= size_t.sizeof, then the closure object can be written directly to the context pointer and the function just receives it as the first arg directly.
alias NonAllocating = void delegate() @nogc;
NonAllocating dg;
void fun(T* thing)
{
dg = () { thing.method(); }; // this lambda does not need to allocate a closure
}
// the closure struct would be:
struct Closure
{
T* thing;
}
static assert(Closure.sizeof <= size_t.sizeof);
// the delegate can be created like
Closure closure; // don't allocate it, just pop it on the stack temporarily
closure.thing = thing;
dg.ptr = cast(ref size_t)closure; // dg.ptr stores a copy of the closure as an integer
dg.funcprt = &lambda_fun;
A shim could be implemented like:
void lambda(size_t closure_bits) // first arg is context `ptr` from delegate object
{
// generate: first line in lambda reinterprets the context arg as the closure struct
ref Closure this = cast(ref Closure)closure_bits;
// carry on using context as usual...
thing.method(); // via `this`, in the namespace as usual?
}
Almost every lambda I write only populates the closure with a single item, which means a huge number of lambda's automatically become @nogc and can be used much more broadly.
It's also a nice optimisation for 'normal' D code; reducing some GC churn, which may be meaningful in lambda-heavy code; especially algorithms/pipeline/component programming. I bet this would substantially reduce allocation in algorithm-heavy code.
This would also make it really easy to do one really common operation; promote a function to a delegate:
void function() f;
void delegate() dg;
dg = () => f(); // `f` is the only capture; naturally populates the closure by-value as above
We just promoted a function to a delegate with no allocation and no hacky/funny business!
To clarify, this is a problem with @nogc and not LDC (in)ability promote allocations to the stack?
🤨
It's not really either of those things; just that small closures should never allocate. There are many advantages.
This might work for const or head-const pointers (like this), but in general, keeping other data in the context pointer of a delegate changes semantics if the delegate is copied and passed around. The closure data is then copied aswell, but should only be referenced.
Yeah, it was discussed up-thread that it only works if it's head-const. Does the compiler have enough knowledge to know if a value is ever written?