Add SharedAllocatorList by jercaianu · Pull Request #6465 · dlang/phobos

jercaianu · 2018-04-19T19:48:53Z

Added the shared version of AllocatorList.
This will be very useful together with SharedAscendingPageAllocator. Instead of mapping a huge chunk of virtual memory when constructing the allocator, we can now use it together with SharedAllocatorList and gradually map smaller chunks of memory on demand.

dlang-bot · 2018-04-19T19:48:54Z

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

n8sh

Some things about it seem a bit odd to me, but they're the same as the existing AllocatorList so I'll give this a thumbs up.

edi33416

LGTM

wilzbach

LGTM too.
I would prefer if we could automatically generate all this locking code with static foreach over the public methods of the base type (D shouldn't be about repeating code).
At the moment we can't generate documentation strings (I think someone was working on a DIP for it, but probably that got stalled).

Also most doc strings miss the Returns/Params ddoc parts.

wilzbach · 2018-05-02T08:58:22Z

std/experimental/allocator/building_blocks/allocator_list.d

+    auto allocators()
+    {
+        return impl.allocators;
+    }


Do we really need this? Can't you use impl.allocators directly?

One could, but this allows us to keep our unittests DRY. Many of them test both, the shared and the regular versions. The regular version has a private field named allocators which is what this accessor functions emulates. Removing this helper function would make our unittests quite WET.

Furthermore, this function is both private and version (unittest). I don’t think there’s any harm done by keeping it in.

wilzbach · 2018-05-02T08:59:12Z

std/experimental/allocator/building_blocks/allocator_list.d

+        lock.lock();
+        scope(exit) lock.unlock();
+
+        return assumeUnshared(impl).allocate(s);


Could be a one-liner:

synchronized(lock) return assumeUnshared(impl).allocate(s);

wilzbach · 2018-05-02T09:13:02Z

std/experimental/allocator/building_blocks/allocator_list.d

+    The alignment offered.
+    */
+    enum alignment = impl.alignment;
+


FWIW all the following code could be replaced with a nice static foreach.
Of course the disadvantage is that it won't be shown in the docs, which actually might not be too important.

That static foreach got implemented, then reverted later.
https://github.com/dlang/phobos/compare/27fa9abb91cd563075958411c00817245e7b6c15..f91e9e7e77b3f3e023ab96241389636d12cef03d

Given this PR already had an “auto-merge” label after that reverting, I’d say, we keep it as-is for now. In case someone wants to opt for the clever meta programming loop, they are free to send their PR.

wilzbach · 2018-05-02T09:17:34Z

std/experimental/allocator/building_blocks/allocator_list.d

+    creates a new allocator by calling `make(s)` and delegates the request
+    to it. However, if the allocation fresh off a newly created allocator
+    fails, subsequent calls to `allocate` will not cause more calls to $(D
+    make).


Params need to be documented.

To be fair, nobody felt that need for the regular AllocatorList from which this comment was copied.

After having a look at similar function in the package, I saw their common denominator was to not have their parameters documented. And this could have almost been on purpose – have a look at the package documentation of “building blocks” that provides a table documentating each of these functions in general.

phobos/std/experimental/allocator/building_blocks/package.d

Line 52 in b0f461b

$(TR $(TDC void[] allocate(size_t s);, $(POST $(RES) is null || $(RES).length ==

Apart from that, the text exactly describes the parameter meaning.

wilzbach · 2018-05-02T09:17:45Z

std/experimental/allocator/building_blocks/allocator_list.d

+    satisfy the request, creates a new allocator by calling `make(s + a - 1)`
+    and delegates the request to it. However, if the allocation fresh off a
+    newly created allocator fails, subsequent calls to `alignedAllocate`
+    will not cause more calls to `make`.


Params + Returns

wilzbach · 2018-05-02T09:17:58Z

std/experimental/allocator/building_blocks/allocator_list.d

+    turn, in most-recently-used order. If the owner is found, it is moved to
+    the front of the list as a side effect under the assumption it will be used
+    soon.
+


wilzbach · 2018-05-02T09:18:08Z

std/experimental/allocator/building_blocks/allocator_list.d

+    /**
+    Defined only if `Allocator.expand` is defined. Finds the owner of `b`
+    and calls `expand` for it. The owner is not brought to the head of the
+    list.


Params + Returns

I haven’t been able to find a single expand() function in Phobos with such documentation. Therefore, I consider this out-of-scope for this PR.

wilzbach · 2018-05-02T14:49:36Z

@jercaianu try sth. like this:

static foreach (T; __traits(allMembers, typeof(impl)))
{
    static if (mixin("isCallable!(impl." ~ T ~ ")") && !T.startsWith("__"))
    static if (mixin("__traits(getProtection, impl." ~ T ~ ") == `public`"))
    {
        mixin(`auto ` ~ T ~ `(Parameters!(impl.`~T~`) params)
        {
            lock.lock();
            scope(exit) lock.unlock();
            return assumeUnshared(impl).`~T~`(params);
        }
        `);
    }
}

wilzbach · 2018-05-02T14:53:45Z

Oh and if you use synchronized you don't need assumeUnshared because the compiler is then smart enough:

synchronized(mutex) {
  return impl.foo();
}

(though it needs to be a mutex, e.g. new Mutex)

andralex

@RazvanN7 can you please look at the compiler bug exposed by the win32 failure?

andralex · 2018-05-21T15:45:26Z

std/experimental/allocator/building_blocks/allocator_list.d

+    */
+    enum alignment = impl.alignment;
+
+    static foreach (T; __traits(allMembers, typeof(impl)))


Please add a comment before this loop explaining what it does. Cool idea @wilzbach !!

AFAICT this loop got removed. Nothing left to comment.

andralex · 2018-05-21T15:46:41Z

std/experimental/allocator/building_blocks/allocator_list.d

+            `);
+        }
+    }
+}


Note: this does not work well with rvalues because it copies them once more. Probably not a problem here because all parameters are cheap.

I’ve added this note as a comment. See: d701391
If it really affects someone they can open an issue or send a patch that changes it, I suppose.

rebase

thewilsonator · 2022-01-14T11:30:27Z

Removing auto merge, this is stalled.

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

@thewilsonator this one can be merged with resolutions.

thewilsonator · 2024-12-26T06:17:48Z

Rebased as #10594

0xEAB · 2024-12-27T00:46:10Z

Oh and if you use synchronized you don't need assumeUnshared because the compiler is then smart enough:
[…]
(though it needs to be a mutex, e.g. new Mutex)

I’m reluctant to change this now. This PR had already got its “auto-merge” label as-is.

If someone feels like a mutex is a better fit, they sure can send a follow-up PR.
Alternatively, I would happily take this into consideration with #10594, if someone submitted a thorough change request.
(But I’m not making this decision on my own – which would overrule Razvan’s “auto-merge” – when I have no expertise to back it up with.)

jercaianu requested review from andralex, edi33416, n8sh and wilzbach April 19, 2018 19:49

n8sh approved these changes Apr 24, 2018

View reviewed changes

edi33416 approved these changes Apr 25, 2018

View reviewed changes

wilzbach approved these changes May 2, 2018

View reviewed changes

jercaianu force-pushed the shared_allocator_list branch 3 times, most recently from 1d4dc09 to 27fa9ab Compare May 21, 2018 12:54

andralex approved these changes May 21, 2018

View reviewed changes

jercaianu force-pushed the shared_allocator_list branch from 27fa9ab to f91e9e7 Compare May 23, 2018 14:33

dlang-bot added Merge:Needs Rebase Merge:stalled labels May 27, 2021

Add SharedAllocatorList

b0f461b

rebase

RazvanN7 force-pushed the shared_allocator_list branch from f91e9e7 to b0f461b Compare January 13, 2022 14:46

dlang-bot removed Merge:Needs Rebase Merge:stalled labels Jan 13, 2022

RazvanN7 approved these changes Jan 13, 2022

View reviewed changes

RazvanN7 added the Merge:auto-merge label Jan 13, 2022

dlang-bot added the Merge:stalled label Jan 13, 2022

thewilsonator removed the Merge:auto-merge label Jan 14, 2022

dlang-bot removed the Merge:stalled label Jan 14, 2022

dlang-bot added the Merge:stalled label Apr 14, 2022

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

0xEAB mentioned this pull request Dec 26, 2024

Add SharedAllocatorList #10594

Merged

thewilsonator removed the Review:Salvage This PR needs work, but we want to keep it and it can be salvaged. label Dec 26, 2024

thewilsonator closed this Dec 26, 2024

Uh oh!

Conversation

jercaianu commented Apr 19, 2018

Uh oh!

dlang-bot commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

n8sh left a comment

Choose a reason for hiding this comment

Uh oh!

edi33416 left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented May 2, 2018

Uh oh!

wilzbach commented May 2, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented Jan 14, 2022

Uh oh!

LightBender commented Oct 27, 2024

Uh oh!

thewilsonator commented Dec 26, 2024

Uh oh!

0xEAB commented Dec 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dlang-bot commented Apr 19, 2018 •

edited

Loading