[WIP] Aa lower lib - work in progress by somzzz · Pull Request #1985 · dlang/druntime

somzzz · 2017-12-07T13:43:20Z

dlang-bot · 2017-12-07T13:43:21Z

Thanks for your pull request and interest in making D better, @somzzz! 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.

schveiguy · 2017-12-07T15:02:07Z

ping @quickfur as I think you attempted something like this at one point.

andralex

OK, so did you get to build successfully with these changes?

andralex · 2017-12-07T17:53:00Z

src/assoc_array.d

@@ -0,0 +1,1046 @@
+/**
+ * Implementation of associative arrays.


s/Implementation/Templated implementation/

andralex · 2017-12-07T18:26:31Z

src/assoc_array.d

+private enum HASH_DELETED = 0x1;
+private enum HASH_FILLED_MARK = size_t(1) << 8 * size_t.sizeof - 1;
+
+private @property bool empty(in AssocArray!(void, void)* aa) pure nothrow @nogc


Shouldn't all functions in this module be vacuous templates so as to force on-demand compilation? I mean:

private @property bool empty()(in AssocArray!(void, void)* aa) pure nothrow @nogc

andralex · 2017-12-07T18:26:50Z

src/assoc_array.d

+}
+
+
+bool hasPostblit(in TypeInfo ti)


same here etc

andralex · 2017-12-07T18:27:40Z

src/assoc_array.d

+    return ti;
+}
+
+struct AssocArray(K, V)


Great, the methods inside this struct don't need to be vacuous templates because they already are templates as members of a template struct.

andralex · 2017-12-07T18:28:02Z

src/assoc_array.d

+    }
+    return ti;
+}
+


Please insert a comment explaining what AssocArray is and does.

quickfur · 2017-12-07T23:33:36Z

I haven't looked into this in detail yet, but from a cursory glance, I have the following comments:

The current usage of AssocArray!(void,void)* seems to be strange and questionable. Isn't the whole point of a templated AA implementation to take advantage of compile-time type information?
And to that point, the continued usage of TypeInfo seems to indicate that this implementation isn't that different from the one we already have now. So where does the templated part come in? By that, I don't mean the mere fact that there are templated parameters, but how they are benefitting the code in ways the current implementation can't do. If all we're doing is to tack on template parameters for the key and value types, and then cast that away and revert to using TypeInfos, then this new implementation is, frankly, pointless. We already have a perfectly fine TypeInfo-based implementation now. The whole thrust behind a templated AA type is to get rid of the dependency on TypeInfos. The key and value types of the AA are already known at compile-time, we should be taking advantage of that, instead of waiting until runtime to inspect the TypeInfo and make decisions then.
If done carelessly, a templated AA type is going to introduce massive amounts of template bloat into existing code, which is Not Nice. Much of the AA implementation doesn't actually (need to) depend on the specifics of the key/value types, e.g., the rehashing algorithm, the collision resolution algorithm, the management of the main table of buckets, etc.. Only a few places actually need to know the specifics of the type: when computing a hash value, when comparing keys, when storing a key / value in a bucket, and when returning the result of a lookup. Ideally, these two parts should be separately implemented; the type-agnostic code should be in a core, non-templated submodule that provides the low-level primitives, and the templated part should be primarily in the user-facing side of things (probably in object.d). Doing this will probably entail a radically different code organization than what is currently there, even if the underlying algorithms will pretty much be the same. (I don't recommend trying to also replace the algorithms, the scope of the code change will be too large to handle in that case, and too prone to breakage. Let's get the templated implementation working first, then we can worry about improving the algorithms.)
Another thing that might be worth considering in a template AA implementation is the possibility of compile-time evaluation. This may be a long shot, but in the ideal case, it would be very nice if AA literals that don't involve runtime values can be completely constructed at compile-time and stored directly in the object file. Then we can have static immutable AA's that don't incur runtime initialization overhead.

andralex · 2017-12-08T00:33:30Z

The current usage of AssocArray!(void,void)* seems to be strange and questionable. Isn't the whole point of a templated AA implementation to take advantage of compile-time type information?

This is a WIP on a difficult goal and we decided to first go through a proof of concepts stage whereby AssocArray!(void,void)* stands in for the current dynamically-typed associative array. Later on we'll get more fancy.

And to that point, the continued usage of TypeInfo seems to indicate that this implementation isn't that different from the one we already have now.

Indeed. That is intentional. We want to make sure the lowering generates correct calls to methods of a template type.

So where does the templated part come in? By that, I don't mean the mere fact that there are templated parameters, but how they are benefitting the code in ways the current implementation can't do. If all we're doing is to tack on template parameters for the key and value types, and then cast that away and revert to using TypeInfos, then this new implementation is, frankly, pointless. We already have a perfectly fine TypeInfo-based implementation now. The whole thrust behind a templated AA type is to get rid of the dependency on TypeInfos. The key and value types of the AA are already known at compile-time, we should be taking advantage of that, instead of waiting until runtime to inspect the TypeInfo and make decisions then.

Agreed. Yes, the plan is to move to a non-Typeinfo implementation.

If done carelessly, a templated AA type is going to introduce massive amounts of template bloat into existing code, which is Not Nice.

We'll indeed look forward to improvements once we get the templated basics in place.

Another thing that might be worth considering in a template AA implementation is the possibility of compile-time evaluation. This may be a long shot, but in the ideal case, it would be very nice if AA literals that don't involve runtime values can be completely constructed at compile-time and stored directly in the object file. Then we can have static immutable AA's that don't incur runtime initialization overhead.

I think we already have that (which may cause difficulty to this project).

andralex · 2017-12-08T00:33:43Z

@somzzz please rebase

quickfur · 2017-12-08T22:11:08Z

@andralex Thanks for clarifying. Using AssocArray!(void,void) as a transitional measure to bridge to the current state of things is not a bad idea. Hoping to see the final results of this effort!

rainers · 2017-12-09T11:03:51Z

You should use some git rename command for rt.aaa.d -> assoc_array.d to not lose history and make it a lot easier to review actual changes.

rainers · 2017-12-09T11:05:15Z

src/assoc_array.d

+}
+
+//==============================================================================
+// API AssocArrayementation


test replacement error

rainers · 2017-12-09T11:07:49Z

src/assoc_array.d

+ *      If key was not in the aa, a mutable pointer to newly inserted value which
+ *      is set to all zeros
+ */
+extern (C) void* _aaGetY(AssocArray!(void, void)** aa, const TypeInfo_AssociativeArray ti, in size_t valsz,


maybe use ref AssocArray!(void, void)* aa here to avoid repeating (*aa) below. Or assign it to some local.

rainers · 2017-12-09T11:14:35Z

src/object.d

+/*
+public template AssocArray(K, V)
+{
+    import assoc_array;


Good to see the import being delayed here. I'd recommend to put assoc_array into some sub package of core or core.internal, though.

I'd insist on that.

somzzz added 2 commits December 4, 2017 07:05

aaLength template

f358869

wip

d8f8d8f

somzzz requested review from Burgos, MartinNowak, andralex, rainers, schveiguy and wilzbach as code owners December 7, 2017 13:43

somzzz changed the title ~~Aa lower lib - work in progress~~ [WIP] Aa lower lib - work in progress Dec 7, 2017

dlang-bot added the WIP Work In Progress - not ready for review or pulling label Dec 7, 2017

andralex reviewed Dec 7, 2017

View reviewed changes

rainers reviewed Dec 9, 2017

View reviewed changes

rt.aaa.d -> assoc_array.d

bcdf290

dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work labels Jan 1, 2018

somzzz closed this Feb 15, 2018

somzzz reopened this Feb 15, 2018

edi33416 added a commit to edi33416/druntime that referenced this pull request Feb 4, 2019

[WIP] Revamp dlang#1985 - Aa lower lib

3595544

edi33416 mentioned this pull request Feb 4, 2019

[WIP] Revamp #1985 - Aa lower lib #2483

Closed

dlang-bot added the stalled label May 27, 2021

RazvanN7 closed this Nov 3, 2021

		@@ -0,0 +1,1046 @@
		/**
		* Implementation of associative arrays.

Uh oh!

Comments

Conversation

somzzz commented Dec 7, 2017

Uh oh!

dlang-bot commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

schveiguy commented Dec 7, 2017

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!

Choose a reason for hiding this comment

Uh oh!

quickfur commented Dec 7, 2017

Uh oh!

andralex commented Dec 8, 2017

Uh oh!

andralex commented Dec 8, 2017

Uh oh!

quickfur commented Dec 8, 2017

Uh oh!

rainers commented Dec 9, 2017

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dlang-bot commented Dec 7, 2017 •

edited

Loading