Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

add parallel marking to the GC#2514

Merged
dlang-bot merged 7 commits intodlang:masterfrom
rainers:gc_parallel2
Apr 29, 2019
Merged

add parallel marking to the GC#2514
dlang-bot merged 7 commits intodlang:masterfrom
rainers:gc_parallel2

Conversation

@rainers
Copy link
Copy Markdown
Member

@rainers rainers commented Mar 22, 2019

I can make the commits separate PRs if it is desired, but this is why events and unmanaged threads are needed.

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request, @rainers!

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 fetch digger
dub run digger -- build "master + druntime#2514"

@thewilsonator
Copy link
Copy Markdown
Contributor

I think it would be good to split out adding @nogc to the windows functions into a separate PR as that is trivial. Ditto for fixing the abort message in src/core/sync/mutex.d.

@rainers
Copy link
Copy Markdown
Member Author

rainers commented Mar 22, 2019

I think it would be good to split out adding @nogc to the windows functions into a separate PR as that is trivial. Ditto for fixing the abort message in src/core/sync/mutex.d.

Done: #2515, #2516

@kinke
Copy link
Copy Markdown
Contributor

kinke commented Mar 22, 2019

This reduces pause times for a collection considerably.

Do you have any hard numbers?

@rainers
Copy link
Copy Markdown
Member Author

rainers commented Mar 22, 2019

Do you have any hard numbers?

Unfortunately most druntime benchmarks do not use much memory so GC activity is only a fraction of the overall runtime, so it doesn't show much difference there.
With a modified gcbench/vdparser-test (more iterations and keeping every second pass in memory) the sequential version prints:

win32: GC summary: 1099 MB,   11 GC 1672 ms, Pauses 1366 ms <  572 ms
win64: GC summary: 1355 MB,   13 GC 1386 ms, Pauses 1044 ms <  394 ms

and the parallel version:

win32: GC summary: 1099 MB,   11 GC  676 ms, Pauses  371 ms <  145 ms
win64: GC summary: 1355 MB,   13 GC  631 ms, Pauses  289 ms <  100 ms

Thats on a i7-4790K (quad-core + HT) @ 4GHz.

@rainers rainers added the WIP Work In Progress - not ready for review or pulling label Mar 30, 2019
@rainers
Copy link
Copy Markdown
Member Author

rainers commented Mar 30, 2019

I've added WIP as there is one known scenario that I don't have a solution yet: if running as a DLL, the GC is terminated during PROCESS_DLL_DETACH, but won't be able to stop the GC threads as they block on the Windows loader lock for DllMain.

Maybe the GC should default to disable parallel marking to avoid breakage.

@rainers
Copy link
Copy Markdown
Member Author

rainers commented Mar 31, 2019

Rebased on top of #2533 to avoid the freeze in the Windows DLL tests.

@rainers
Copy link
Copy Markdown
Member Author

rainers commented Apr 21, 2019

Rebased on top of #2570 and added workaround for missing module dependency (core.cpuid not initialized yet).

@thewilsonator
Copy link
Copy Markdown
Contributor

thewilsonator commented Apr 26, 2019

Is this good to go?

@rainers
Copy link
Copy Markdown
Member Author

rainers commented Apr 26, 2019

I this good to go?

I think so, but I'm not 100% sure if parallel marking should be on by default. There is a chance that some programs that use little memory might get a little slower, mainly due to the possibly only (and dumb) default collection at the very end that might start threads and then has to wait for these to terminate. Any opinions?

@rainers rainers removed the WIP Work In Progress - not ready for review or pulling label Apr 26, 2019
@thewilsonator
Copy link
Copy Markdown
Contributor

I suggest making sure that the feature is sufficiently advertised, have it on by default for a release and see what the reaction is.

Having it on by default would mean any kinks would get reported quickly.

The final collection is configurable too, right? I guess the user can adapt the configuration to suit their needs. Does the GC know if the collection it is doing is to shutdown? It shouldn't be too hard to not spawn a bunch of thread if that is the case. We can fix that later if it is needed.

@thewilsonator thewilsonator added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 26, 2019
@rainers
Copy link
Copy Markdown
Member Author

rainers commented Apr 26, 2019

The final collection is configurable too, right?

Yes, it is --DRT-gcopt=cleanup:none.

Does the GC know if the collection it is doing is to shutdown?

No, only during DLL shutdown ATM. Thread creation fails in that case anyway.

Having it on by default would mean any kinks would get reported quickly.

Good. I'll make a PR for the documentation...

@thewilsonator
Copy link
Copy Markdown
Contributor

Good. I'll make a PR for the documentation...

Thanks.

@thewilsonator thewilsonator added auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Apr 29, 2019
@dlang-bot dlang-bot merged commit 47b03c1 into dlang:master Apr 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants