Make AffixAllocator take into account RCISharedAllocator #6205

edi33416 · 2018-02-21T10:58:31Z

No description provided.

dlang-bot · 2018-02-21T10:58:32Z

Thanks for your pull request and interest in making D better, @edi33416! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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 fetch digger
dub run digger -- build "master + phobos#6205"

wilzbach · 2018-02-23T01:28:34Z

std/experimental/allocator/building_blocks/affix_allocator.d

+
+@system unittest
+{
+    import std.experimental.allocator : RCISharedAllocator, processAllocator;


processAllocator isn't used here ;-)

wilzbach · 2018-02-23T01:33:17Z

std/experimental/allocator/building_blocks/affix_allocator.d

+
+    AffixAllocator!(RCISharedAllocator, uint) a;
+    auto buf = a.allocate(42);
+    assert(buf.length == 42);


Any chance we can replace the processAllocator with something like this?

auto prevAllocator = processAllocator; scope(exit) processAllocator = prevAllocator; alias SCAlloc = StatsCollector!(Mallocator, Options.bytesUsed); SCAlloc statsCollectorAlloc; processAllocator = allocatorObject(statsCollectorAlloc); ... assert(statsCollectorAlloc.bytesUsed == 100);

wilzbach · 2018-02-23T01:35:50Z

std/experimental/allocator/building_blocks/affix_allocator.d

@@ -69,11 +70,21 @@ struct AffixAllocator(Allocator, Prefix, Suffix = void)
    static if (stateSize!Allocator)
    {
        Allocator _parent;
-        static if (is(Allocator == RCIAllocator))
+        static if (is(Allocator == RCIAllocator) || is(Allocator == RCISharedAllocator))


This behavior isn't mentioned in the docs above...

edi33416 · 2018-03-28T13:44:11Z

std/experimental/allocator/building_blocks/affix_allocator.d

@@ -69,11 +70,21 @@ struct AffixAllocator(Allocator, Prefix, Suffix = void)
    static if (stateSize!Allocator)
    {
        Allocator _parent;


I think this, Allocator _parent, should be made private as we want the user to use the parent() method.

edi33416 · 2018-03-28T16:50:33Z

@jercaianu

edi33416 · 2018-03-28T16:52:29Z

I've moved the parent() logic inside the mixin template Impl as this also needs to infer the shared attribute.

jercaianu

Aside from some minor comments, great job!

jercaianu · 2018-03-28T19:41:54Z

std/experimental/allocator/building_blocks/affix_allocator.d

-                if (_parent.isNull) _parent = theAllocator;
-                assert(alignment <= _parent.alignment);
-                return _parent;
+                Allocator parent()


Does this need to return by value?
Shouldn't it return a reference to the parent?

jercaianu · 2018-03-28T19:44:07Z

std/experimental/allocator/building_blocks/affix_allocator.d

+                        }
+                        else
+                        {
+                            _parent = processAllocator;


Since parent can be called by multiple threads, is this assignment guaranteed to be thread safe?

jercaianu · 2018-03-28T19:46:10Z

std/experimental/allocator/building_blocks/affix_allocator.d

@@ -520,3 +540,30 @@ struct AffixAllocator(Allocator, Prefix, Suffix = void)
    static assert(is(typeof(a.allocate) == shared));
    assert(buf.length == 10);
 }
+
+@system unittest


I think it would be a good idea to also have a test in which we access this from multiple threads.

edi33416 · 2018-04-26T15:34:05Z

@n8sh could you have another look at the code? Changed sync to use only one variable

n8sh · 2018-06-13T14:46:22Z

std/experimental/allocator/building_blocks/affix_allocator.d

+                                    (&wrapProcAllocatorObject))();
+                            _parent = fn();
+                        }
+                        parentUnlock(_lockVar);


To avoid deadlock instead put scope(exit) parentUnlock(_lockVar); right after the lock is acquired.

RazvanN7 · 2021-04-29T10:52:08Z

@edi33416 What is the state of this PR? Should we pull it in?

LightBender · 2024-10-27T00:11:26Z

@thewilsonator if can rebase this and get it in we'll take it.
@edi33416 if you want to resurrect this we'll take it in.

wilzbach reviewed Feb 23, 2018

View reviewed changes

edi33416 commented Mar 28, 2018

View reviewed changes

edi33416 force-pushed the aff_all_rcshared branch from 41bf8dd to d4cf839 Compare March 28, 2018 16:47

edi33416 requested review from andralex and PetarKirov March 28, 2018 16:49

jercaianu reviewed Mar 28, 2018

View reviewed changes

n8sh approved these changes Apr 3, 2018

View reviewed changes

n8sh added the Needs Rebase label Apr 3, 2018

edi33416 force-pushed the aff_all_rcshared branch from d4cf839 to 5877da2 Compare April 25, 2018 21:24

dlang-bot removed the Needs Rebase label Apr 25, 2018

edi33416 force-pushed the aff_all_rcshared branch 2 times, most recently from b03976f to 25c65ab Compare April 25, 2018 22:57

edi33416 added the allocators label Apr 25, 2018

n8sh added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 26, 2018

Make AffixAllocator take into account RCISharedAllocator

8076b6f

edi33416 force-pushed the aff_all_rcshared branch from 25c65ab to 8076b6f Compare April 26, 2018 15:32

n8sh removed the 72h no objection -> merge The PR will be merged if there are no objections raised. label Jun 13, 2018

n8sh reviewed Jun 13, 2018

View reviewed changes

dlang-bot added the Needs Rebase label May 27, 2021

dlang-bot added the stalled label Jul 28, 2021

LightBender added the Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make AffixAllocator take into account RCISharedAllocator #6205

Make AffixAllocator take into account RCISharedAllocator #6205

edi33416 commented Feb 21, 2018

dlang-bot commented Feb 21, 2018 •

edited

Loading

wilzbach Feb 23, 2018

wilzbach Feb 23, 2018

wilzbach Feb 23, 2018

edi33416 Mar 28, 2018

edi33416 commented Mar 28, 2018

edi33416 commented Mar 28, 2018

jercaianu left a comment

jercaianu Mar 28, 2018

jercaianu Mar 28, 2018

jercaianu Mar 28, 2018

edi33416 commented Apr 26, 2018

n8sh Jun 13, 2018

RazvanN7 commented Apr 29, 2021

LightBender commented Oct 27, 2024

Make AffixAllocator take into account RCISharedAllocator #6205

Are you sure you want to change the base?

Make AffixAllocator take into account RCISharedAllocator #6205

Conversation

edi33416 commented Feb 21, 2018

dlang-bot commented Feb 21, 2018 • edited Loading

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edi33416 commented Mar 28, 2018

edi33416 commented Mar 28, 2018

jercaianu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edi33416 commented Apr 26, 2018

Choose a reason for hiding this comment

RazvanN7 commented Apr 29, 2021

LightBender commented Oct 27, 2024

dlang-bot commented Feb 21, 2018 •

edited

Loading