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

std/path: make globMatch work with @nogc #9055

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

Conversation

ljmf00-wekaio
Copy link

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @ljmf00-wekaio! 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#9055"

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Otherwise looks good

std/path.d Outdated Show resolved Hide resolved
std/path.d Outdated Show resolved Hide resolved
@LightBender
Copy link
Contributor

LightBender commented Sep 29, 2024

@jmdavis Brought this to my attention. I think this code points out a fundamental flaw in how we use @nogc both in Phobos and in general library code. Specifically, this code is technically correct. It is @nogc insofar as the GC is no longer called, but if somebody were to ever use this code in their @nogc hot-path code they'd probably be surprised to find out that it's allocating underneath them.

This is because in D, @nogc has two meanings. The first is the technical meaning of "No GC Allocations". The second is a community-semantic meaning of "no allocations". So in one sense, this code is fibbing about what it does. It is technically correct, but for most users, they will see the @nogc and assume that the implementation does not allocate at all, which is incorrect, and they will only be able to determine the truth by actually reading the code, which of course, nobody does.

Of course, we can always fall-back to "you should just know that @nogc doesn't mean no allocations" but this is poor library design as this would require that we document all allocations regardless of allocation method and then tell everybody to RTFM. Furthermore, this code does not meet even the Phobos 3 standards for @nogc code. All @nogc code in Phobos 3 that requires allocation must request a buffer from the user (or an allocator, once we get those).

The overarching problem here is that @nogc has two different meanings that forces us to carefully consider our design choices. The design standards of Phobos 3 are intended to enforce a design where all @nogc methods are explicit about their allocation patterns in the call-site, which means that user is always aware of what they are doing. Given the present situation with @nogc this is likely the only realistic path forward for our @nogc library support.

As this code fails to meet the Phobos 3 design standards I would request that you either redesign this method to meet them or consider providing an alternative implementation via overload.

@CyberShadow
Copy link
Member

Calling a function allocates memory (on the stack).

I think you would need to define what "allocate memory" means more precisely before beginning to discuss whether @nogc allows it or not.

The first is the technical meaning of "No GC Allocations".

Actually that is not quite correct. @nogc means "does not use the GC", which includes other operations beyond allocating GC-owned memory. The point is that @nogc code can be linked into a program which does not include a garbage collector. Note that this is by itself useful in its own way, but quite far away from any perceived meaning of "does not allocate memory".

@jmdavis
Copy link
Member

jmdavis commented Sep 29, 2024

Calling a function allocates memory (on the stack).

I think you would need to define what "allocate memory" means more precisely before beginning to discuss whether @nogc allows it or not.

That's a valid point. We should try to be precise with what we mean. However, I think that it's pretty clear that he meant heap allocations, which is almost always what people mean when talking about allocations. Stack allocations happen all over the place whether you like it or not and aren't avoidable to the point that there's really no point in even discussing them with regards to APIs. The fact that they happen is a given, and they're a non-issue unless you start doing something like creating huge static arrays or run attempt really deep / infinite recursion.

The first is the technical meaning of "No GC Allocations".

Actually that is not quite correct. @nogc means "does not use the GC", which includes other operations beyond allocating GC-owned memory. The point is that @nogc code can be linked into a program which does not include a garbage collector. Note that this is by itself useful in its own way, but quite far away from any perceived meaning of "does not allocate memory".

From the standpoint of actually using @nogc, using it to determine whether you can avoid linking against the GC code is pretty pointless. You'll get that from the linker as soon as you try to link, and anyone looking to avoid the GC to that degree simply won't link in druntime. The whole point of @nogc is to be able to say that no GC allocations occur and that a GC collection can't be triggered, which enables you to reliably use that function in contexts where you can't afford for heap allocations to occur or for a GC collection to trigger. That's done by saying that no GC functions can be called, and in turn, that does technically mean that it will indicate that that function doesn't need symbols from the GC, but if that's your goal, @nogc is completely unnecessary.

@ljmf00-wekaio
Copy link
Author

The second is a community-semantic meaning of "no allocations".

No, it's conceptually wrong and I don't think the community follows that either. The whole reason for GC being costly is because of the entire mechanism of tracking the memory, not the allocation mechanism alone. The same for nothrow but with the argument that the exception unwinding code is expensive.

So in one sense, this code is fibbing about what it does. It is technically correct, but for most users, they will see the @nogc and assume that the implementation does not allocate at all

I think those are mostly your assumptions, at least in current Druntime/Phobos. I can prove that with a simple grep in the code. See

T[] result = (cast(T*) pureMalloc(nbytes))[0 .. len];
. I would accept arguing this should be documented, because documentation is where you clear out assumptions made by the users.

Take another example, where user assumes X or Y method is pretty fast, but turns out the method does a syscall in the background? What about a specific syscall that is potentially more expensive to the user? I think its not sensible to assume that the method does or does not a syscall nor a specific one, unless its documented properly, and I guess the same goes with allocations and other specific behaviour.

Would be cool to have rules for this, like enforce documenting it? Yes, do we have it currently? No. Can we still document it, yes.

Of course, we can always fall-back to "you should just know that @nogc doesn't mean no allocations"

It's literally the meaning of @nogc, usage of GC and nothing about "no allocations". Would you say, with your last argument, then, that mmap being @nogc it doesn't "allocate" at all? No, of course not. Allocation is also a very broad term. It vastly depends on your environment. Like @CyberShadow mentioned, isn't calling every function that uses stack, allocating?

All @nogc code in Phobos 3 that requires allocation must request a buffer from the user (or an allocator, once we get those).

While I agree with the allocations (we need to redesign the current Phobos allocators tho), why you bringing this PR, which changes a Phobos v2 implementation?

The overarching problem here is that @nogc has two different meanings that forces us to carefully consider our design choices

Sorry, but you seem to fabricate the second meaning. There's only one meaning of the @nogc attribute specified in the language documentation. I can see your point but this is not the current situation, at all.

As this code fails to meet the Phobos 3 design standards I would request that you either redesign this method to meet them or consider providing an alternative implementation via overload.

Why are we trying to make Phobos v2 the Phobos v3 already? We can clearly see this has room for debate, but I don't understand why here. The effort here is to avoid calling a GC collection for such trivial method. I could rework the method to not even use any sort of allocation, which, btw, I think that is the most sensible way, but that's beyond this change. This change itself is an improvement over the previous situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants