Replace IAllocator with reference counted struct #5921
Conversation
edi33416
requested review from
PetarKirov,
andralex and
wilzbach
as code owners
|
Thanks for your pull request, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
edi33416
force-pushed
the
rc_iallocator
branch
2 times, most recently
from
f449552 to
7986102
Compare
|
I'll approve this to keep PR size under control. Subsequent work should add attributes to allocator interface appropriately. |
|
There are still three dub projects failing due the breaking changes you introduce, so we have to wait a bit anyhow. Also I feel like this PR is merged a bit too quickly. It still contains debug work and as such a big breaking API change, at least having a motivation in the changelog for why the IAllocator interface needs to be bloated with incRef/decRef would be nice. |
Happy merging & tagging a new release:
CC @s-ludwig |
@edi33416 could you please insert a changelog before going on vacation? |
|
Dont forget to close https://issues.dlang.org/show_bug.cgi?id=15509 |
| /// Ditto | ||
| nothrow @safe @nogc @property void theAllocator(IAllocator a) | ||
| nothrow @system @nogc | ||
| @property void theAllocator(RCIAllocator a) |
There was a problem hiding this comment.
Is it defined whether allocators can always free their backing memory on destruction, or whether they have to leak when there are still outstanding allocations?
I'm assuming the former as the bookkeeping overhead for the latter seems problematic.
If that's the case, making the theAllocator setter unsafe is quite a severe choice.
Sure you correctly force code that replaces the current allocator to be unsafe, but how could that code take any responsibility for all the allocations that were done with the previous theAllocator (or at least for that code to be reasonably auditable).
This would limit the safe Allocator usages to:
- setting
theAllocatoronce on startup - using local allocators for specific tasks
- others???
But excluded (b/c crazily unsafe) those usages:
- exchanging
theAllocatorafter some time to optimize for the current workload - others???
Global allocators with finite lifetime are inherently unsafe. We should have a good answer to how we intend allocators to be used, before making this step.
As a side node, the old void theAllocator(IAllocator a) was already unsafe, as it could eliminate the last GC reference to the previous theAllocator. Allocations don't GC-pin their allocator and aren't usually GC tracked anyhow.
On a lighter note, this is the second special RC type (after RC Throwables), that's ending up in dlang before we're set on how to implement manual memory management in the language. Another class-based RC type instead of a polymorphic RC!(IAllocator). It's good to experiment, but it remains questionable whether std.experimental is the right place ;).
Anyhow, hopefully this won't cause too much maintenance effort down the road.
There was a problem hiding this comment.
But excluded (b/c crazily unsafe) those usages:
- exchanging
theAllocatorafter some time to optimize for the current workload
What if the user wraps his allocator in a StatsCollector allocator that tracks the bytesUsed? This way he could safely change theAllocator
I'm thinking of something like:
alias SCAlloc = StatsCollector!(Mallocator, Options.bytesUsed);
SCAlloc statsCollectorAlloc;
// setup the allocator
() @trusted { theAllocator = allocatorObject(statsCollectorAlloc); }();
// do some work
// check if we can safely change the allocator
if (theAllocator.impl.bytesUsed == 0) { // change theAllocator }
There was a problem hiding this comment.
Nice, good idea.
Haven't thought too well about how that would interact with possible compile-time lifetime checks (for RC et.al.), but keeping track of the outstanding bytes should allow for a cheap runtime safety check before exchanging theAllocator.
Would it be relevant whether the allocator set as theAllocator has been used beforehand? Would this be worthwhile to add as a static introspectable empty method to the allocator interfaces?
For processAllocator keeping track of bytes atomically might be somewhat expensive. Some clever per-thread accounting/caching could lower the overhead there.
There was a problem hiding this comment.
Would it be relevant whether the allocator set as theAllocator has been used beforehand?
Good question. The design is young, and at some point we'll need to answer this.
Would this be worthwhile to add as a static introspectable empty method to the allocator interfaces?
We already have empty: https://dlang.org/phobos/std_experimental_allocator.html#.IAllocator.empty
For processAllocator keeping track of bytes atomically might be somewhat expensive. Some clever per-thread accounting/caching could lower the overhead there.
This is tricky because the very notion of "empty" is evanescent for a shared allocator. You can't check for empty and then change the allocator - that's a race. We'd need some CAS-based mechanism. Let's discuss this further as needs arise.
edi33416
force-pushed
the
rc_iallocator
branch
from
7986102 to
72e46ab
Compare
edi33416
force-pushed
the
rc_iallocator
branch
2 times, most recently
from
95e2b8d to
fdc1a54
Compare
wilzbach
force-pushed
the
rc_iallocator
branch
from
90e3079 to
9a10c02
Compare
|
The diet-ng PR has been merged and is avalable as version 1.4.4 now. |
wilzbach
force-pushed
the
rc_iallocator
branch
from
9a10c02 to
5f87349
Compare
|
Thanks a lot @s-ludwig!
I can't reproduce this locally with |
wilzbach
force-pushed
the
rc_iallocator
branch
2 times, most recently
from
632c303 to
502dcdf
Compare
|
@edi33416 you are in luck. Martin fixed the DUB regression this morning (dlang/dub#1350), so it looks like it's only dlang/ddox#194 (which lets dlang/dub fail) left. |
wilzbach
force-pushed
the
rc_iallocator
branch
3 times, most recently
from
2be85b6 to
a1cb4b0
Compare
|
Hmm, the dub test doesn't want to pick up the new ddox version, e.g. : However, on my local machine (with verbose log) And even though the registry is down at the moment, the mirror picked up 0.16.8 correctly: https://code-mirror.dlang.io/packages/ddox Also at dlang/ci the entire dub cache is purged after every component has run. Has someone an idea how to force the dub testsuite to use the latest |
|
Okay I can reproduce this with so it seems like we need to force dub to do an explicit fetch of void runDdox(bool run, string[] generate_args = null)
{
import std.process : browse;
if (m_dryRun) return;
// allow to choose a custom ddox tool
auto tool = m_project.rootPackage.recipe.ddoxTool;
if (tool.empty) tool = "ddox";
auto tool_pack = m_packageManager.getBestPackage(tool, ">=0.0.0");
if (!tool_pack) tool_pack = m_packageManager.getBestPackage(tool, "~master");
if (!tool_pack) {
logInfo("%s is not present, getting and storing it user wide", tool);
tool_pack = fetch(tool, Dependency(">=0.0.0"), defaultPlacementLocation, FetchOptions.none);
}So in lack of better ideas, I will try to nuke |
wilzbach
force-pushed
the
rc_iallocator
branch
from
a1cb4b0 to
573a85b
Compare
Looks like that didn't work out either and |
Seems to work. |
|
Looks like processAllocator setter doesn't implement proper multithreading - it decrements reference before storing allocator. Also using setter before getter will still trigger initialization in getter. |
|
cc @edi33416 |
|
Ack. Will follow up with a PR to fix this. @d-random-contributor thanks for pointing those out |
7 participants
No description provided.