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

Move block info caching into the gc package #17067

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

schveiguy
Copy link
Member

This is step 1 in migrating the array runtime code into the GC. Once all these steps are completed, the new GC will be able to hook to druntime.

This does not really change how the rt.lifetime array runtime code works. It's just now using the block info caching from core.internal.gc.blkcache.

A couple of things I changed when I did this:

  1. Since the rt code is no longer handling the block cache, I moved the processMarks function out of rt, and therefore the record of the BlkInfo cache record for each thread.
  2. I don't know why I didn't do this before (probably inexperience), but instead of using C malloc to allocate the BlkInfo cache, use a GC malloc with NO_SCAN.
    1. This also simplifies the management of the cache block -- we can just let the GC clean it up after the thread exits instead of having to hook cleanup.
    2. We also do not have to store a pointer to the TLS pointer, we can just store a pointer to the allocated block in the thread.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @schveiguy!

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 + dmd#17067"

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.

Oh, I see this is mostly a cut & paste. Feel free to do those suggestion in a follow up PR.

If you could split out any changes that are not purely cut & paste, e.g. the IsMarked changes like IsMarked isMarked(void *addr) scope nothrow into a separate commit that would be appreciated.

druntime/src/core/internal/gc/blkcache.d Outdated Show resolved Hide resolved
druntime/src/core/internal/gc/blkcache.d Show resolved Hide resolved
druntime/src/core/internal/gc/blkcache.d Outdated Show resolved Hide resolved
druntime/src/core/internal/gc/blkcache.d Show resolved Hide resolved
@schveiguy
Copy link
Member Author

If you could split out any changes that are not purely cut & paste, e.g. the IsMarked changes like IsMarked isMarked(void *addr) scope nothrow into a separate commit that would be appreciated.

I will see if I can figure out how to do this. I agree it would be better.

@schveiguy
Copy link
Member Author

@thewilsonator ok, I split into 2 commits. The first commit just moves the code as-is into the blkcache module. The second commit shows the edits I made in that module (and everywhere else).

split block cache marking functionality from processing of the tls
sections.
@schveiguy
Copy link
Member Author

Is that stdcheader thing my fault? or is that a commonly failing test? Unsure how my code could make that happen...

@thewilsonator
Copy link
Contributor

thewilsonator commented Nov 16, 2024

I guess so, it seems to have hit one of Walter's PRs #17066 (comment)

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.

Apart from that, looks good

@@ -71,6 +71,7 @@ COPY=\
$(IMPDIR)\core\internal\elf\io.d \
\
$(IMPDIR)\core\internal\gc\bits.d \
$(IMPDIR)\core\internal\gc\blkcache.d \
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be part of the first commit

Copy link
Member Author

Choose a reason for hiding this comment

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

These commits were split to aid in review. The first commit is a straight copy/paste of code. In essence, they are not separable -- you can't build with the first commit only, even if I were to make these changes go into the first commit.

I had a hard time doing the split, don't make me do it again...

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@@ -528,6 +528,7 @@ DOCS=\
$(DOCDIR)\etc_linux_memoryerror.html \
\
$(DOCDIR)\core_internal_gc_bits.html \
$(DOCDIR)\core_internal_gc_blkcache.html \
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -74,6 +74,7 @@ SRCS=\
src\core\internal\elf\io.d \
\
src\core\internal\gc\bits.d \
src\core\internal\gc\blkcache.d \
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@schveiguy
Copy link
Member Author

Ugh, first hard problem. Need to figure out why sociomantic code is failing.

@thewilsonator
Copy link
Contributor

I would suggest splitting the first commit off into a separate PR (along with the makefile changes) and then also splitting out the formatting changes.

@schveiguy
Copy link
Member Author

I think I know what the problem is. I have to go back to using malloc instead of the GC for the cache... Detached threads...

@schveiguy
Copy link
Member Author

I would suggest splitting the first commit off into a separate PR (along with the makefile changes) and then also splitting out the formatting changes.

It wouldn't work. The first commit is not buildable.

@thewilsonator
Copy link
Contributor

If its a cut& paste then the only other thing you should need (apart from the makefile changes) would be a public import from where you cut from of where you pasted it, no?

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.

3 participants