Skip to content

FixedArray & Sorted #2793

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

Closed
wants to merge 19 commits into from
Closed

FixedArray & Sorted #2793

wants to merge 19 commits into from

Conversation

Panke
Copy link
Contributor

@Panke Panke commented Dec 10, 2014

This implements two different wrapper structs.

Sorted is designed as a wrapper around a random access range or a container that guarantees that all elements are stored in sorted order. It should offer all possible container primitives, especially those that require log(n) search, like lowerBound, upperBound and equalRange.

FixedArray takes a random access range and offers an interface similar to std.container.Array using this range as storage. This is used by std.container.Sorted and could be used by std.container.BinaryHeap as well.

Neither FixedArray nor Sorted use RefCounted to keep track of it's storage, see and answer
http://forum.dlang.org/thread/uapqmdbtadpwnciwfdwn@forum.dlang.org
if you don't agree.

@nordlow
Copy link
Contributor

nordlow commented Dec 10, 2014

It's a little shorter to use

enum isRAContainer(T) = isRandomAccessRange!(typeof(T.init[])) ...

Array type that uses a random access range as fixed storage location.
No memory is ever allocated. Throws if not enough space is left
for an operation to succeed. */
struct FixedArray(Store)
Copy link
Contributor

Choose a reason for hiding this comment

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

If Store is a random-access range, it should have a template constraint asserting it.

@JakobOvrum
Copy link
Contributor

In the past I was working on something like this myself, that was more generic, designed to work with non-RA containers and conditionally support linear or logarithmic operations depending on the underlying container. See this thread for some discussion on this topic (as well as a link to what I had, if you're interested).

this(Store s, size_t initialLength = size_t.max)
{
_store = s;
_length = min(initialLength, _store.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Random-access ranges can be infinite and thus not have length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think infinite ranges make sense as storage so I put another constraint in.

@andralex
Copy link
Member

OK I read through FixedArray and it has a number of strategic and tactical issues.

The basic idea of FixedArray is a container capped to a predetermined capacity using a chunk of preallocated storage as its backend. I think the notion has merit.

One important matter is that all containers must have reference semantics, i.e. assigning a container object to another must refer to the same payload. The current implementation stores a range of unknown behavior and the effective length together, which is definitely not going to work regardless of what the range does.

A good thing to do (which will become safer soon) is to use reference counting. It's more work but I think containers are terrific candidates for reference counting; they are relatively large and have "personality", i.e. they're more like entity objects that stick around rather than small temporary objects.

A few tactical matters concern construction and destruction of object. The container object assumes the range has been initialized to capacity, which may be overkill. Then it never destroys elements when the length gets shorter. All that needs to be fixed with care.

@Panke
Copy link
Contributor Author

Panke commented Jan 18, 2015

Except for the destruction of contained objects, all your concern should already be non-issues if Array!T is used as the underlying store and for the rest of my answer, I'll assume that Store is something (anything?) that passes isRandomAccessRange!Store.

Furthermore I don't think that Sorted (and BinaryHeap for that) should as adaptors take ownership of its underlying store. IMO this should be the responsibility of the store.

One important matter is that all containers must have reference semantics, i.e. assigning a container object to another must refer to the same payload.

Yes and I think the current implementation already does this with the majority of range implementations out there. Though I concede that you can write a random access range, where this implementation will fail. Especially if the range implements postblit to copy the contents of the range.

One solution to this would be to simply require that every Store must have reference semantics itself. Another one to take the range by ref in the constructor and store an pointer to it internally (or use Store*) as argument directly. Last but not least we could move the Store to some allocated heap space ourself and use RefCounted or the garbage collector.

Thinking about it, I'd say we should use RefCounted in the general case and use the current implementation if we can determine that it's actually not needed. That would be the case if the Store is a std.container or a dynamic array.

Edit: Correcting the code I realized that the length always needs to be shared, too. So it's RefCounted, now.

A few tactical matters concern construction and destruction of object. The container object assumes the range has been initialized to capacity, which may be overkill.

Maybe requiring that the Store is at least as initialized as a std.array.minimallyInitializedArray is a good compromise. After all the implementation does not take a arbitrary memory slice, but a container/range with correct element type, using its length not its capacity as upper bound.

Then it never destroys elements when the length gets shorter. All that needs to be fixed with care.

Yes we should do that.

@andralex
Copy link
Member

OK, I can get behind this. There are a few things to do, some of which are unnecessary work caused by my being confused. Apologies about that.

So FixedArray's charter is: Install a random-access container interface on top of a preallocated random-access range. I'll go with that semantics, which among other things means parts of the existing API needs to be removed (as I'll note in follow-up comments).

  • I'll only discuss FixedArray here. As first order of business, please break this pull request into two (one for each of FixedArray and SortedArray). They are different enough and large enough to warrant separate discussions.
  • Rename FixedArray to PreallocatedArray. The latter is more descriptive because the array is in fact not fixed in size, just has been preallocated.
  • Move both PreallocatedArray and SortedArray to std.experimental. That's the matter of course for new library artifacts.
  • Since the store has been preallocated, the array has no business allocating and deallocating stuff. That means the method .dup must go away. If anyone wants to duplicate an array, they'd fetch the range inside, duplicate it however they find fit, and then install a PreallocatedArray on top of it.
  • Somewhat sadly RefCounted should stay because the current length of the array must be shared among array instances.
  • Documentation should state it expects each element of the range to be safely initialized, e.g. with .init. This is needed because e.g. when the garbage collector enters in action, it should be able to use the type of the underlying store to clean things up properly.
  • After destructing any element (e.g. remove from tail), PreallocatedArray should obliterate its content with .init, again so it leaves the store in a collectible state.

I'll make one more pass with details.

@@ -0,0 +1,504 @@
module std.container.fixed_array;

import core.exception, std.algorithm, std.conv, std.exception, std.range,
Copy link
Member

Choose a reason for hiding this comment

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

Can most of these imports be migrated inside the struct? That way the imports won't be effected unless someone uses it.

@Panke
Copy link
Contributor Author

Panke commented Jan 30, 2015

Opened #2939

@Panke Panke closed this Jan 30, 2015
@Panke Panke mentioned this pull request Jan 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants