Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add to PhobosV3 memory allocators from ProjectSidero #9078

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rikkimax
Copy link
Contributor

@rikkimax rikkimax commented Oct 30, 2024

I got a bit sick of people saying allocators won't be coming.

So here we go, here are my ~2 year old allocators.

Some key differences from PhobosV2:

  1. If something is there, it is complete.
  2. Alignment is a property of the allocator, not something you can request.
  3. Struct only for the allocator (I would love to use classes, but no RC support).
  4. No shared allocator, either it is appropriate to use or you are in a specialized use case and shouldn't be going virtual.
  5. Uses void[] everywhere.
  6. Annotated for positive annotation (export) and negative annotation (@hidden).

I did have to add some utility modules such as my test test and set lock, and add Ternary to typecons.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @rikkimax! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#9078"

@rikkimax rikkimax force-pushed the allocators branch 2 times, most recently from de6460e to a4f0b18 Compare October 30, 2024 16:58
Copy link
Contributor

@pbackus pbackus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of @trusted on functions that may free aliased memory is unsound. These functions must be @system.

phobos/sys/allocators/mapping/malloc.d Outdated Show resolved Hide resolved
phobos/sys/allocators/mapping/mmap.d Outdated Show resolved Hide resolved
phobos/sys/allocators/mapping/virtualalloc.d Outdated Show resolved Hide resolved
phobos/sys/allocators/mapping/virtualalloc.d Outdated Show resolved Hide resolved
@rikkimax
Copy link
Contributor Author

Okay, me Paul and Jonathan are in agreement about allocators must be entirely @system, i.e. use after free and double free errors are examples of why it has to be.

It is an example of why I recommend using containers (even dynamic array) to manage memory, rather than interact with an allocator directly.

The next question that needs an answer to as this has been asked in relation to v2, is related to the GC allocator being @nogc. I have it setup along with the rest of library as @nogc. From a practical stance, this will offer the best user experience, I see no reason to remove @nogc from anywhere.

@jmdavis
Copy link
Member

jmdavis commented Oct 31, 2024

The next question that needs an answer to as this has been asked in relation to v2, is related to the GC allocator being @nogc. I have it setup along with the rest of library as @nogc. From a practical stance, this will offer the best user experience, I see no reason to remove @nogc from anywhere.

I don't know what the GC allocator is doing or how you could possibly mark it as @nogc, but if it actually uses GC functions or interacts with the GC in any way, then it should most definitely not be @nogc (and I would think that at least some of its functions would be interacting with the GC if it's a GC allocator). If the compiler will let whatever a function is doing be marked @nogc, then that should be fine (and I could easily see it being the case that some of the functions don't actually use the GC), but if you're doing anything like casting to @nogc, then I think that that's completely inappropriate. As a general rule, if you have to cast to change any attributes or force it via bindings or whatnot, then that's an enormous red flag (e.g. IMHO, it was a huge mistake to try to have versions of malloc or free which were pure; it's a bug factory in the making). If the compiler agrees that an attribute is correct for a piece of code, then great, but if we have to do anything to force it, then we should be very reticent to do so, and we should be extremely careful about it in the rare case where we decide to do so.

We're also going to have to have discussions on how we want to use @nogc in general, since it's clear that a lot of people make incorrect assumptions about it on a regular basis (like that it can't allocate). However, in the case of allocators, they're obviously allocating, and aside from an allocator that allocates with the GC, they presumably aren't going to be using the GC, and folks are going to want to use them in @nogc code, so @nogc would be appropriate. But any function that actually uses the GC, should not be marked with @nogc.

In any case, I will say that while I'm fine with giving this a cursory glance, I'm not going to spend the time to review it in detail (and honestly, it's way too much code to review all at once). Personally, I think that if you ever need allocators, you're in a very dark place where I do not want to be, and I don't want to see them used in Phobos' API anywhere outside of the actual allocator modules - and I'd be perfectly fine with them not existing in Phobos at all. I don't feel so strongly about that that I'm going to fight to keep them out of Phobos (and clearly, there are folks who very much want them), but if we start doing like C++ did and templating stuff on allocators, I think that we have a serious usability problem. As it is, those template arguments are almost never used in C++.

Either way, because I would avoid allocators like the plague, I'm likely not the right person to be making decisions on what exactly their API should look like. I can comment on general language issues, but if an issue is truly allocator-specific, other people (like Paul) are far better qualified than I am. Personally, I think that outside of very niche code, it's a mistake to use allocators (though there are certainly some high performance use cases where they're needed).

@LightBender
Copy link
Contributor

My thoughts.

First. This is an absolutely gargantuan PR. @rikkimax, is there any way we can split this up in to more digestible chunks? While on the topic splitting things up, would it be possible to split Ternary out into a separate PR? typecons is next on our list so it would be a timely addition.

Second. Allocators have been a perennially contentious topic, with some like @jmdavis being of the opinion that we should not even have them because they are so specialized. And if they are agreed to be included, I am sure that the community has deeply held opinions on what they should look like. @pbackus I know this is a big ask, but would it be possible to review @rikkimax's design and determine if it is suitable for Phobos and how it compares to the existing design? It does not need to be a detailed code review, just enough to get an understanding of it. I know that the existing design has it's flaws and I think the community will be most curious to hear about how this design stacks up against it.

Third. Once the first two items are done, we can open this up to the community for discussion on the design, detailed code review, and whether or not we should even have allocators. This is a huge component and I would feel uncomfortable merging this without a very thorough community discussion and review process.

@rikkimax
Copy link
Contributor Author

Ternary can certainly be split out.

As for contents, phobos.sys.allocators.stats, phobos.sys.allocators.alternatives and phobos.sys.allocators.mapping have 1:1 comparable implementation in v2, and a subset of phobos.sys.allocators.buffers have comparable implementation in v2, although what is in there may not be complete in v2. phobos.sys.allocators.api is a subset of the interface stuff, i.e. no aligned allocation or IAllocator vs ISharedAllocator.

phobos.sys.allocators.locking, and phobos.sys.allocators.utils is new.

From my perspective, there isn't a whole lot that specifically needs looking at in terms of appropriateness of inclusion. As long as phobos.sys.allocators.api is correct, then its just a matter of verify that everything at the scope level is where it needs to be.

We can improve and fix bugs later.

@pbackus
Copy link
Contributor

pbackus commented Oct 31, 2024

Just looking at phobos.sys.allocators.api, it seems like the main differences from std.experimental.allocator are:

  1. Fewer methods. For example, both alignedAllocate and expand are missing.
  2. Reference counting is part of the allocator API. Individual allocators must opt in to reference counting by implementing the refAdd and refSub methods.
  3. Thread-safety relies on a NeedsLocking flag instead of shared. Not a huge deal given the current state of shared, but something we might regret as the language continues to evolve.

@pbackus
Copy link
Contributor

pbackus commented Oct 31, 2024

Re: should Phobos 3 have allocators.

Just as the purpose of the range API is to serve as a common interface between generic algorithms and sources of data, the purpose of an allocator API is to serve as a common interface between generic containers and sources of memory.

So, instead of asking "should we have allocators," what we should ask is, "should we have allocator-aware containers"?

Given the current state of the D language (i.e., ignoring unreleased/experimental features), it is impossible to implement generic containers that are both (a) @safe, and (b) allocator-aware. I have written about why this is the case in exhaustive detail here and here.

Personally, if we can only choose one, my vote would go to @safe, which means I would vote no on allocators in Phobos 3.

@rikkimax
Copy link
Contributor Author

Given the current state of the D language (i.e., ignoring unreleased/experimental features), it is impossible to implement generic containers that are both (a) @safe, and (b) allocator-aware. I have written about why this is the case in exhaustive detail here and here.

In neither article do you go into the fact that there is a subset of data structures that can use allocators and be @safe.

Specifically if the data structure does not allow access to internal memory (at least in @safe code) and does not offer deallocation of pointer values on removal.

The path forward in my view, is to have containers that are classified in behavior, where that behavior is well defined. Some may not be implementable in D today with allocators, and others may lend themselves to GC only, that's fine, we can do both.

@pbackus
Copy link
Contributor

pbackus commented Oct 31, 2024

In neither article do you go into the fact that there is a subset of data structures that can use allocators and be @safe.

Specifically if the data structure does not allow access to internal memory (at least in @safe code)

I take it as given that Phobos 3's containers must allow by-ref access to their elements. If our containers are missing important features, it doesn't matter how @safe they are; nobody will want to use them.

@rikkimax
Copy link
Contributor Author

In neither article do you go into the fact that there is a subset of data structures that can use allocators and be @safe.
Specifically if the data structure does not allow access to internal memory (at least in @safe code)

I take it as given that Phobos 3's containers must allow by-ref access to their elements. If our containers are missing important features, it doesn't matter how @safe they are; nobody will want to use them.

Some of my data structures do not offer this intentionally and that is a key characteristic for their usage.

We can have multiple variations of a data structure, we do not need to gatekeep based on the perceived requirements of a different variation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phobos 3 The PR is for Phobos V3. Review:Needs Approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants