Skip to content

std.typecons.RefCounted, std.container.array.Array, & similar structs that manage their own memory do not need to be scanned unless GC-allocated memory is reachable through them#7923

Closed
n8sh wants to merge 1 commit intodlang:masterfrom
n8sh:issue-21775

Conversation

@n8sh
Copy link
Member

@n8sh n8sh commented Mar 28, 2021

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 28, 2021

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
21775 enhancement std.typecons.RefCounted, std.container.array.Array, & similar structs that manage their own memory do not need to be scanned unless GC-allocated memory is reachable through them

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7923"

@PetarKirov
Copy link
Member

PetarKirov commented Mar 28, 2021

I suggest using UDA as the API for type author to override the default policy:

import core.memory : GC;

@(GC.ShouldScan.no)
struct MyType
{
    // ...
}

@RazvanN7
Copy link
Collaborator

@n8sh Please add the word "Fix" at the beginning of your commit message so that the bot understands that you are actually fixing the issue.

@PetarKirov
Copy link
Member

PetarKirov commented Mar 31, 2021

BTW, we should check if hasIndirections(T) can be optimized via https://dlang.org/spec/traits.html#getPointerBitmap, as IIRC @rainers added this trait, because the equivalent recursive template implementation was way too slow to compile. Also we may consider integrating the handling of GC.ShouldScan UDA inside getPointerBitmap.

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Apr 6, 2021

ping @n8sh

@n8sh
Copy link
Member Author

n8sh commented Apr 11, 2021

@RazvanN7 recently I was traveling. Updated commit message as requested.

@Imperatorn
Copy link
Contributor

Would be nice with a short description what it does and why it's needed. Like pseudo

Why? Improve performance... etc
How? Annotate types with uda to enable... etc

@n8sh
Copy link
Member Author

n8sh commented Apr 24, 2021

I suggest using UDA as the API for type author to override the default policy:

import core.memory : GC;

@(GC.ShouldScan.no)
struct MyType
{
    // ...
}

@PetarKirov Is there a way to do this conditionally from inside the definition of the struct?

@n8sh
Copy link
Member Author

n8sh commented Apr 24, 2021

Would be nice with a short description what it does and why it's needed.

@Imperatorn did you see the comment above shouldGCScan?

Code that manually allocates memory without using the GC is responsible for calling GC.addRange if there is the possibility the memory might contain a pointer through which GC-allocated memory is reachable. Conservatively determining this via std.traits.hasIndirections can lead to false positives when nesting multiple types that manage their own memory (for instance a refcounted array of refcounted items). This template addresses that problem by acting like std.traits.hasIndirections but adding an escape hatch: any struct or union that defines enum bool __GC_NO_SCAN = true is regarded as having no pointers that can be used to reach GC-allocated memory. Names beginning with two underscores are reserved for use by the language so there is not a danger of conflicting with an enum that is identically-named by coincidence.

Bear in mind that this template is not public so the explanation is written for Phobos developers.

@thewilsonator
Copy link
Contributor

Is there a way to do this conditionally from inside the definition of the struct?

If MyType is a template you can do

template Mytype(T)
{
    alias whatever = logic goes here;
    @whatever
    struct MyType { ... }
}

not sure about for a non-template

… & similar structs that manage their own memory do not need to be scanned unless GC-allocated memory is reachable through them
@RazvanN7
Copy link
Collaborator

RazvanN7 commented Jul 5, 2021

@n8sh is it possible that this could fix this issue: dlang-community/libdparse#387 (comment) ?

@RazvanN7
Copy link
Collaborator

@n8sh how should we proceed here?

@RazvanN7 RazvanN7 closed this Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants