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

Add SharedAllocatorList #6465

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jercaianu
Copy link
Contributor

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
Copy link
Contributor

dlang-bot commented Apr 19, 2018

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"

Copy link
Member

@n8sh n8sh left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@edi33416 edi33416 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

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.

auto allocators()
{
return impl.allocators;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

lock.lock();
scope(exit) lock.unlock();

return assumeUnshared(impl).allocate(s);
Copy link
Member

Choose a reason for hiding this comment

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

Could be a one-liner:

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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).
Copy link
Member

Choose a reason for hiding this comment

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

Params need to be documented.

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`.
Copy link
Member

Choose a reason for hiding this comment

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

Params + Returns

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.

Copy link
Member

Choose a reason for hiding this comment

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

Params

/**
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.
Copy link
Member

Choose a reason for hiding this comment

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

Params + Returns

@wilzbach
Copy link
Member

wilzbach commented May 2, 2018

@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
Copy link
Member

wilzbach commented May 2, 2018

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)

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

@andralex andralex left a comment

Choose a reason for hiding this comment

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

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

*/
enum alignment = impl.alignment;

static foreach (T; __traits(allMembers, typeof(impl)))
Copy link
Member

Choose a reason for hiding this comment

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

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

`);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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

@thewilsonator
Copy link
Contributor

Removing auto merge, this is stalled.

@dlang-bot dlang-bot removed the stalled label Jan 14, 2022
@LightBender LightBender added the Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024
@LightBender
Copy link
Contributor

@thewilsonator this one can be merged with resolutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Salvage This PR needs work, but we want to keep it and it can be salvaged. stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants