-
-
Notifications
You must be signed in to change notification settings - Fork 426
Conversation
0856519
to
a1876c8
Compare
@MartinNowak this PR need a you review. New AA has tested with this AA implementation. Passed All druntime, phobos tests and dmd tests (except tests, which disabled in D-Programming-Language/dmd/3904). |
6dddd15
to
4802209
Compare
@MartinNowak ping |
OK, will review it in the next days, but I'm quite busy right now :(. |
OK.
Type deduction is performed by compiler now. I add a type deduction code for future use. |
Well opIndex can either assign or a construct, but this could be done in a UDT as well. I worked on the compiler side (see here) and my biggest issue was that the type of an AA literal is also inferred from the LHS of an assignment. static assert(is(typeof(["a": 0, "b": 1, "c": 2]) == int[string]));
ubyte[string] aa = ["a": 0, "b": 1, "c": 2]; |
Yes, i understood. I simly want to implement AA stuff with minimal compiler changes, to reduce atomic changes. This way potentially reduces regression count before the next release. |
What does a stage of your libraryAA now? Does it passes most of the tests? (Yes, multiply index, aa.string of and similar will be failed, but what about other tests)? |
That's easy, |
We just need a way for the compiler to pass the data and I think
|
Don't you mean |
This PR is not about aaLiteral, but about AssociativeArray.
My version allows aa literal with different types of key and value. In can be used for containers like JSONObject: |
It actually doesn't matter for now, so long as it works. So long as it is completely internal to the compiler+runtime, we can change it at will. Implement whatever is easiest. |
My current implementation is the easiest, as I think. However, the next our goal is the implementing AA as library type and @MartinNowak suggests to do a next step now. |
@MartinNowak ping.
|
True that. |
We can also reuse |
this.next = next; | ||
} | ||
|
||
this(K, V)(size_t hash, const(K)* key, const(V)* value, Entry* next = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass key and value by ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remembered why I pass arguments by pointer.
In some places (like line 561) I need to cast argument from const to mutable. When I cast l-value, it becomes r-value: e.key
is l-value, cast(Key)&e.key
is r-value. Pointers helps avoid this trouble.
New literal syntax implementation can has a pitfalls. For example:
In this plan, each item depends on previous and in should be executed in series. |
return prime_list[i]; | ||
} | ||
return prime_list[$-1]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prime stuff is unnecessary when using a proper hash. I'd favour a pow-2 hashtab as in rt.util.container.hashtab
, which is simpler and cheaper to reshape.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if it turns out to be too much work, leave it aside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. BTW. Why core.stdc.log2 isn't marked as pure
?
This change causes small phobos issue: D-Programming-Language/phobos/pull#2549
Yes, you're right. That still means we'd possibly break working code, no idea how to solve it though. |
I would point out that The situation is kind of sucky... |
This implementation uses template-based hash calculation, thus you may use your class as key (in pure code), if your static type of your class declares pure toHash:
|
OK, I was under the impression you could not change purity on overriding, but maybe that's only if you go from pure to impure. |
Ok. However our main goal is a choosing and implementing dmd part. What you have objections against my implementation? |
I was working on the dmd part, but the AA implementation doesn't work with non-copyable values. unittest
{
static struct NC
{
@disable this(this);
}
AssociativeArray!(int, NC) aa;
aa[0] = NC();
aa[1] = NC();
aa[0] = NC(); // new behavior!!!, should throw unless D_NoBoundsChecks is set
} Also RValues aren't moved into the AA in the most efficient way, and it's the extra copy that makes the compiler complain about the disabled postblit. |
Please note, non-copyable values are always RValue assignments (the crappy aaGetX API won't let you know). So for copyable RValues we can't do an efficient assignment, but for non-copyable values we have to. |
See IgorStepanov#1. |
That's actually quite a bummer. Makes me even wonder whether the whole vtable idea is good. |
How about this? |
Hmm, I think, users will not have much motivation to test new AA implementation instead of built-in aa. |
You worry too much because of this (non-copybale elements). Current soluthion follows a existing implementation. We may add simple bitwise copy for it (as temporary hack).
Still, I would suggest to speed up the process and introduce new AA as possible faster, because it better than existing AA in many respects and I do not see any disadvantages in comparison with the old implementation. Anyway I sure that if this disadvantages will be found, we will able quickly fix them. In other words if we introduce this implementation then nobody gets hurt. Thus there are not reasons to delay this. |
They'll have a very high motivation to do this, the implementation will be faster and literals can be put into the data segment.
Do you have a list of those? I know that
We would first have to add a newAA flag.
This pull uses vtables and generates code for each template instance, that's already worse. On top of that it suddenly disallows things where the old implementation was simply sloppy. This will break code which is really worse. These would be my requirements for a good generic AA implementation:
With the vtable approach we can only achieve 1, 5, and 6 and the dmd pull (https://github.com/D-Programming-Language/dmd/pull/4175/files) already adds a lot of code that is specific to the vtable implementation but wouldn't work with a full library type. So it looks as if we might get stuck with a 3 out of 6 implementation that comes with 3 drawbacks. |
The main difference is a multiple-indexing issue:
This code can't be implemented in a library type using current opIndex rules.
Ok, agree with this point, that's little bit worse. (10%)
This will not break any correct code. If you tell about
If AA hole would be acceptable, dmd spec would say something like
This is a hole which must be closed as soon as possible. If someone abused this hole, he himself to blame.
I think type-system correctness and ctfe-friendliness is sufficient reasons.
This is really breakage the existing code unlike this work:)
1, 2, 5, 6 are already done (with the new
Only one row:
The my main argument: this implementation doesn't break any correct existing code. If someone forgot to add However, the decision is yours, I'll do as you say. |
15b8790
to
355bb77
Compare
@MartinNowak Please, tell your decision about this. |
Sorry for being late, it's a really tough decision.
Ah right remember that one. Any known clever hacks to implement it?
Redundant copying yes, but not redundant postblitting.
Correct or not is the wrong metric, "it currently compiles and works but broke with the new release" is the right one. This is a basic type, the fallout of such changes with be immense.
DMD spec is not the best authority, get's updated every once in while when someone filles a Bugzilla and makes a pull.
Sorry to say, but that's the wrong attitude. We're trying to become a more professional language and as such cannot afford to flush working production code down the toilet, because of something that someone should have read.
Both are indeed very good ones.
Not really, because the affected code might be in a different library. Those code breakages start to cascade very badly since we have dub. I was recently baffled by the fact that vibe.d would only build on 2.066.1 (see here).
It'd be really unfortunate to have 2 AA types, but it'd be the same with a newAA switch.
Gradually breaking code isn't better, because we'd have to break things several times. |
I was hoping for a much better AA implementation, but this is merely the same as the old rt.aaA, now even more complicated. Regarding our recent brainstorming about open addressing. |
Just A Moment. Looks like we made noise from scratch. Current dmd part doesn't check attribute correctness. Yes, aaLiteral instance may be impure, throwable or system. However, we doen't create CallExp and don't call semantic for it. We simply get a aaLiteral back-end symbol and manually generate calling code for it. Thus no changes and no breakage now.
There was only one redurant postblit call case: when you pass struct literals to aaLiteral.
To sum up: there no breakage for redurant postblit calls and attributes. I don't see another breakage cases. If you see it, please publish it and I'll thing how we can fix it.
Starting from this point, over two months, ping to me will more debt than before. However I don't leave this and will do work when I'll have a time.
Unfortunately, even if we implement full library-defined AA, it will be complicated, because we should handle different hard cases like non-copybale keys or values. |
@MartinNowak another one ping :) |
Thanks for your effort on this Igor and sorry for realizing do late that it wouldn't work out. Let me close that pull for now, it might have given us enough insight to succeed the next time. |
@MartinNowak
|
@MartinNowak ping |
We were at this point already were we had 2 different AA types in the compiler, it was a mess.
You can't do that in the
I don't question the value of that, but it likely still adds a lot code/complexity/maintenance for one specific goal.
You can use a specialized hash table that supports perfect hashing for that. |
There is some code duplication, but it is not two different AA. We will need only two aa literal constructors: one for runtime (existing
I know, I told simplistically.=) We are partialy done it in my implementation.
Of course, I can do it, but the native support of ctfeble AA will be better.
Awesome! |
The idea is to provide a much better alternative while slowly pushing people out of the semantic special behavior of the built-in AA. But before that can happen we need AA literals for library types. |
Wait what? You've actually given up on matching the builtin AA semantics? Why? |
Because of things like |
If we can add that to the language, fine, but the attribute changes alone require a slow transition. |
This PR introduces new AA implementation. At the first stage, compiler will not know about
core.internal.aa.AssociativeArray
and will use only object.aaNew and object.aaLiteral methods.Thus, this AA implenetation should replace rt.aaA implemetation.
When first stage will be finished, we will start work to endowing
core.internal.aa.AssociativeArray
with all the abilities of language's AA. For example:However, now I dont's talk about second stage. I want to finish first stage, wait one release cycle (to catching all possible troubles), and start to work under second stage when the next D version (after first stage finish) will be released.