Split DList Node into DNode and PayNode. #2457

monarchdodra · 2014-08-23T08:25:21Z

This splits the "Nodes" in DList into two types of node:

The "base" DNode that has no payload.
The "derived" PayLoad: A DNode with a payload.

Doing this helps avoid allocating a T for no reason in the sentinel node. It should also remove some template bloat, as a few functions actually had no dependency on the type carried by the payload.

@DmitryOlshansky : What do you think?

DmitryOlshansky · 2014-08-23T08:30:27Z

std/container/dlist.d

+    //Converts a DNode into a PayNode
+    private static inout(PayNode)* asPayNode(inout(DNode)* p) @trusted pure nothrow
+    {
+        return cast(typeof(return))p;


Hackish but I guess we have to leave with this.

The only issue with this approach, is safety. I marked it as trusted, but truth be told it really just isn't safe.

That said, currently everything is GC allocated, but if and when we move to allocators, the entire container will probably seize to be safe.

Might as well not make any eager false promises?

DmitryOlshansky · 2014-08-23T08:32:50Z

Overall - nice. Anybody else?

monarchdodra · 2014-08-23T11:58:36Z

I made an indentation change due to a private block. Suggest viewing with ?w=1.

monarchdodra · 2014-08-28T18:39:46Z

I'm having second thoughts about this: The issue is that it introduces an unsafe cast. If somebody where to call "front" (to assign) on an empty DList in a release build, then that's clobbering someone else's memory => unsafe.

Furthermore, (currently), DList uses the GC, so it is possible to use a DList in a safe context. This pull would break this.

Supposedly, if and when we migrate to an allocator DList, then all of DList will become unsafe anyways. But until then, it's a gratuitous breaking change (IMO).

With all that said, I believe having a base non-template structure is a move in the right direction. But for the purposes of safety, I still think that the sentinel node should be allocated as a paynode, and with a payload.

@DmitryOlshansky : What are your thoughts on this? Honestly (IMO), a little extra allocation for safety is acceptable. If you are creating a list, it implies you will want a fair amount of items. I don't think 1 extra item is the end of the world?

The alternatives is either making the list @system here and now (could actually be a good move in regards to breaking things now rather than later). Or always check that the container is not empty, when array bounds checking is on (not quite the same variable as "release").

Also, @andralex , I think your input would be relevant?

quickfur · 2014-08-29T20:27:32Z

std/container/dlist.d

@@ -3,6 +3,83 @@ module std.container.dlist;
 import std.exception, std.range, std.traits;
 public import std.container.util;

+/+
+A DList Node without payload. Used to alocate the sentinel node (henceforth "sentinode").


Ultra-nitpick: "allocate" instead of "alocate".

quickfur · 2014-08-29T20:44:42Z

I really like this PR. It reduces overhead for the sentinel node, but more importantly it cuts down template bloat, potentially by a lot if you have many DLists of different item types. So I'd like to push this through if possible.

As for calling .front on an empty DList, I'm almost tempted to say it's undefined behaviour. But in any case, making DList unusable in @safe code is Not Nice. I hope we can find a solution to that.

DmitryOlshansky · 2014-08-29T21:05:52Z

@monarchdodra I share your reservations about braking @safe.
Regarding undefined .front - well putting an assert here or boundscheck is IMHO a simple way to amend the problem.

The main gain is like @quickfur puts it in saving on template bloat. To that end there is a lot of unlocked potental - hoisting out functionality that takes DNode. It feels like a DList!void may provide a common basis. This is far reaching goal though and is a lot like what C++ STL has been doing (e.g. hoisting out allocator dependency out of iterators), I do not think current pull should aim for perfect antibloat.

quickfur · 2014-08-29T21:39:05Z

I like the idea of Dlist!void being the baseline. I think this is a worthwhile pattern to pursue in general in std.container. Though, of course, beyond the scope of this PR.

As for .front, there is already an assert for non-emptiness in .front. What @monarchdodra is talking about, is the fact that this assert is not present in release builds, which would imply that @safety is compromised because now there's a real possibility of corrupting memory, which violates @safe, whereas in non-release builds it will abort right before then.

quickfur · 2014-08-29T23:27:27Z

Whoa, that was a totally random unrelated autotester failure. Some kind of race condition in std.file unittests? Or perhaps one of those nasty NFS / VM FS synchronization issues?

mihails-strasuns · 2014-08-30T03:26:08Z

Concept looks yummy. Figure out the auto-tester thing and it is LGTM

quickfur · 2014-08-30T03:33:55Z

I don't think the autotester failure has anything to do with this PR. It's probably an unrelated random breakage. But we shouldn't merge until we have a satisfactory solution for the @safe breakage.

mihails-strasuns · 2014-08-30T03:38:54Z

But we shouldn't merge until we have a satisfactory solution for the @safe breakage.

What is the problem with replacing it with enforce!SomeErrorType?

quickfur · 2014-08-30T03:41:03Z

Good idea. Maybe we should just do that, then we can merge.

mihails-strasuns · 2014-08-30T03:43:14Z

You can even do enforce!AssertError to emulate an assertion that is guaranteed to stay in release builds (though I'd recommend some more meaningful error type)

monarchdodra · 2014-08-30T18:17:26Z

OK, here is a status of the pull:

I've reverted to having the sentinel allocated as a normal node with payload. This is the behavior we had before, so it's not any worst than we used to have it. Rather, let's just assume that this pull is now "only" about phasing out non-template code out of template code.

Let's start with this step. Is that OK?

My rationale for this is:

I still believe some extra allocation is better than extra complexity and run-time checks. Maybe it's just me, but as I said, it's how it used to be.
I could just be implementation leak. For example, DList.Range would remain safe either way, since it references the first and last nodes, but never the sentinel. Maybe there are ways to tweak the implementation to make it a non-issue? I'd need more time to think about it.

I like the idea of Dlist!void being the baseline.

I guess, or just DListBase or something. I have not yet put any thought on this. One issue is that there is actually incredibly little code in DList itself that would warrant it. I'd have to study it.

DmitryOlshansky · 2014-08-31T09:50:29Z

@monarchdodra Insertion/Deletion of nodes is exactly the same code for any kind of node. Neither of operation touches payload nor needs to know there is one. The only place where payload s needed is accessors like .front

DmitryOlshansky · 2014-08-31T09:51:09Z

I've reverted to having the sentinel allocated as a normal node with payload. This is the behavior we had before, so it's not any worst than we used to have it. Rather, let's just assume that this pull is now "only" about phasing out non-template code out of template code.

Let's start with this step. Is that OK?

Yes, let's go with that.

monarchdodra · 2014-08-31T11:44:21Z

Insertion/Deletion of nodes is exactly the same code for any kind of node. Neither of operation touches payload nor needs to know there is one.

Well, yes and no. You still have to allocate the node, or potentially destroy it. That asside, it could work yeah.

I had taken the approach to do it via a free function "connect", but doing it via a base struct could probably be cleaner. I'll give it a try.

DmitryOlshansky · 2014-08-31T11:49:34Z

Well, yes and no. You still have to allocate the node, or potentially destroy it. That asside, it could work yeah.

Easy to abstract away by taking a DNode function() construct.
More interestingly construction of a payload node should happen before insertion. Then we pass a pointer to the "head/base" part of node as DNode to the insertion function.

DmitryOlshansky · 2014-08-31T11:52:08Z

In other words - a one simple DListBase datastructure that speaks only in DNode and does all of List operations should be good enough basis for typed payload version.

monarchdodra · 2014-08-31T12:29:37Z

In other words - a one simple DListBase datastructure

Yeah, that's what I had in mind. I'll do it in this pull.

monarchdodra · 2014-08-31T18:02:06Z

I'll do it in this pull.

Wow, this is actually going incredibly well. I'm real happy about how it's going.

One issue I'm running into though is the new code is non-template, which means it is pre-compiled, so all asserts in the base class are stripped away in the distributed build. Even if they are placed in contracts. I'm tempted to just say "fuck it" and purposefully add more of the issue in Phobos. It'll put more pressure on DMD to do something about the issue...

quickfur · 2014-09-03T19:46:17Z

ping
Are your latest changes in? I'd like to get this merged, as a good precedent for what future container templates should look like.

monarchdodra · 2014-09-03T19:55:07Z

Are your latest changes in? I'd like to get this merged, as a good precedent for what future container templates should look like.

...maybe. The changes of adding a DListBase are almost finished, but the changes are actually quite involved. I think it would be better to do it in two steps, so as to merge any "obvious" improvements quick, and then debate on a bigger pull. I'll ping when done.

quickfur · 2014-09-03T21:31:51Z

Sounds good, thanks!

quickfur · 2014-09-19T17:51:45Z

std/container/dlist.d

+
+    private this(BaseNode* first, BaseNode* last)
+    {
+        assert(!!first == !!last, "Dlist.Range.this: Invalid arguments");


What's the reason for writing !!first instead of first !is null?

Legacy I guess :) . That said, it can be simplified to a single !, since we just want to compare binary states. Which do you prefer:

assert(!first == !last, "Dlist.Range.this: Invalid arguments"); or assert(first is null == last is null, "Dlist.Range.this: Invalid arguments");

I prefer the latter, since it makes the intent clear. (Assuming, of course, that the compiler is smart enough that this produces identical machine code.)

I'll change it then.

quickfur · 2014-09-19T17:59:12Z

Overall, looks good.

DmitryOlshansky · 2014-09-20T08:09:47Z

std/container/dlist.d

@@ -820,3 +900,12 @@ unittest //13425
    r = list.linearRemove(r.take(1));
    assert(r.empty); // fails
 }
+
+@safe unittest //check safety


Not really sure what this tries to do.

What do you mean? It's a safe unittest. It makes sure the code inside that unittest is useable in a safe context. It might be smarter to simply add the tag to existing unittests that actually do things though...

monarchdodra · 2014-09-20T13:44:28Z

Fixed ! and unitest safety. I did uncover https://issues.dlang.org/show_bug.cgi?id=13508 , but it's not blocker.

quickfur · 2014-09-20T21:42:32Z

LGTM.

mihails-strasuns · 2014-09-21T14:35:57Z

LGTM, though I'll wait for Dmitry before merging :)

Split DList Node into DNode and PayNode.

monarchdodra · 2014-09-22T20:20:57Z

Thanks. I'm a bit busy, so I don't know how soon I can get to doing the "base DList" thing though. I'll do it eventually, just not in an immediate future.

monarchdodra · 2014-09-24T12:09:35Z

@DmitryOlshansky : #2553

CyberShadow · 2015-03-18T12:34:27Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=14300

CyberShadow · 2015-10-30T11:21:28Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15263

DmitryOlshansky reviewed Aug 23, 2014
View reviewed changes

quickfur reviewed Aug 29, 2014
View reviewed changes

quickfur reviewed Sep 19, 2014
View reviewed changes

DmitryOlshansky reviewed Sep 20, 2014
View reviewed changes

Split DList Node into DNode and PayNode.

f51f8d4

DmitryOlshansky added a commit that referenced this pull request Sep 21, 2014

Merge pull request #2457 from monarchdodra/DNode

0cf3b3c

Split DList Node into DNode and PayNode.

DmitryOlshansky merged commit 0cf3b3c into dlang:master Sep 21, 2014

Split DList Node into DNode and PayNode. #2457

Split DList Node into DNode and PayNode. #2457

Conversation

monarchdodra commented Aug 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitryOlshansky commented Aug 23, 2014

monarchdodra commented Aug 23, 2014

monarchdodra commented Aug 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quickfur commented Aug 29, 2014

DmitryOlshansky commented Aug 29, 2014

quickfur commented Aug 29, 2014

quickfur commented Aug 29, 2014

mihails-strasuns commented Aug 30, 2014

quickfur commented Aug 30, 2014

mihails-strasuns commented Aug 30, 2014

quickfur commented Aug 30, 2014

mihails-strasuns commented Aug 30, 2014

monarchdodra commented Aug 30, 2014

DmitryOlshansky commented Aug 31, 2014

DmitryOlshansky commented Aug 31, 2014

monarchdodra commented Aug 31, 2014

DmitryOlshansky commented Aug 31, 2014

DmitryOlshansky commented Aug 31, 2014

monarchdodra commented Aug 31, 2014

monarchdodra commented Aug 31, 2014

quickfur commented Sep 3, 2014

monarchdodra commented Sep 3, 2014

quickfur commented Sep 3, 2014

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

quickfur commented Sep 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monarchdodra commented Sep 20, 2014

quickfur commented Sep 20, 2014

mihails-strasuns commented Sep 21, 2014

monarchdodra commented Sep 22, 2014

monarchdodra commented Sep 24, 2014

CyberShadow commented Mar 18, 2015

CyberShadow commented Oct 30, 2015