Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Provide more TypeInfo for AA allocations #1037

Merged
merged 1 commit into from
Jan 23, 2015

Conversation

rainers
Copy link
Member

@rainers rainers commented Nov 22, 2014

This is needed for precise scanning of heap objects allocated by the AA implementation.

  • add _aaGetY with full type info
  • supply more type info to _aaValues, _aaKeys
  • return normal dynamic array, not "raw" array

I'm not sure how obsolete this becomes with the new AA implementation, though.

- supply more type info to _aaValues, _aaKeys
- return normal dynamic array, not "raw" array
@IgorStepanov
Copy link
Contributor

@rainers Why do we need to touch the old implementation? >.<
We have a fully implemented new AA without typeinfo. We need just rewiew #934, dlang/dmd#3904 and small integration PR which I'll create when previous two will be merged. This work has been tested on all platforms and ready to review and merge. Why do we need to spend time on temporary solutions, if possible to put in place proper implementation within next week?

@rainers
Copy link
Member Author

rainers commented Nov 22, 2014

@rainers Why do we need to touch the old implementation? >.<

AFAICT the existing dmd patch only deals with literals, not with insertions. If you have further patches, please make them available somewhere.
Will the new version obsolete aaa.d and respective code generation in the compiler?

We have a fully implemented new AA without typeinfo.

Precise scanning needs the typeinfo for allocated objects. I guess these are easily available from a templated solution, though.

@IgorStepanov
Copy link
Contributor

AFAICT the existing dmd patch only deals with literals, not with insertions.

Current patch touch literal and insertions (_aaGetX), but the new implementation has another philosophy with the old. New AA is template and doesn't use typeinfo. It abuses most of old aaXXX api, but size and typeinfo arguments are ignored.

@IgorStepanov
Copy link
Contributor

The main points of the new AA:

  1. rt.aaA is not needed and will be removed.
  2. V[K] is now represenred by core.internal.aa.AssociativeArray!(K, V)
    3.I plan to implement lowering from V[K] to core.internal.aa.AssociativeArray!(K, V) during semantic, but now compiler works with AA through API functions and doesn't know AA implementation type.
  3. core.internal.aa.AssociativeArray object stores a special "handler" object (similar to class vtbl). It provides accessors to AA. For example new _aaGetRvalueX extracts pointer to vtbl from AA pointer and call pseudo-virtual AA method which takes only void* pointer to key (AA knows own key type and value type, because it template).
  4. When we initializing the new AA object (via literal or via first opIndexAssign) we have an null AA pointer and can't access to vtbl. Thus we need to change compiler: it should create AA literal via aaLiteral template function (which knows key and value types) and pass special factory function to aaGetX (which constructs an empty AA object when paa points to null). This changes implemented in dmd#3904

Precise scanning needs the typeinfo for allocated objects.

It may be easyly implemented, because AA is template. You may add another function to AA vtbl, which returns GC info.

@rainers
Copy link
Member Author

rainers commented Nov 22, 2014

The main points of the new AA:

Sounds good. Please note that the previous try to do this often caused unresolved symbols, because the lowering was only halfway complete, e.g. if you never used an AA literal or insertion, get functions will never have the corresponding template instantiated. Your pseudo-virtual handle might cleverly avoid that problem, though.

@IgorStepanov
Copy link
Contributor

Now it passes dmd, phobos and druntime tests. Looks ike I've caught all the linkage fleas.

@MartinNowak
Copy link
Member

Looks ike I've caught all the linkage fleas.

Well the linkage issues usually arised in other libraries, not in druntime or phobos :).

@MartinNowak
Copy link
Member

Auto-merge toggled on

MartinNowak added a commit that referenced this pull request Jan 23, 2015
Provide more TypeInfo for AA allocations
@MartinNowak MartinNowak merged commit f0c1e13 into dlang:master Jan 23, 2015
@schveiguy
Copy link
Member

Looks like this causes a problem with shared AA: https://issues.dlang.org/show_bug.cgi?id=14192

If I print ti.toString() inside _aaGetY, it prints:

shared(shared(int)[int])

But ti is statically typed as TypeInfo_AssociativeArray. I think we have to check whether it's TypeInfo_Shared or not first. I don't know if any other wrappers would be something needed to check, I don't understand fully the rules for how dmd calls these.

@rainers
Copy link
Member Author

rainers commented Feb 17, 2015

We could fix it by using taa->nullAttributes() instead of taa->mutableOf() in https://github.com/D-Programming-Language/dmd/blob/master/src/e2ir.c#L4593. Or will _aaGetYhave to work differently for shared AAs?

It seems _d_assocarrayliteralTX could also be affected as it also just uses mutableOf. I'm not sue if you can create a shared AA literal, though.

@schveiguy
Copy link
Member

"in" doesn't work any differently for shared AA I don't think. But that's not necessarily going to be true forever. I think that dmd really should be passing the full type info, and letting the runtime deal with peeling the layers off.

@MartinNowak
Copy link
Member

If you make a link to a line reference a commit instead of master, or the link will soon point somewhere else. Github has a handy keyboard shortcut y for this.
https://help.github.com/articles/getting-permanent-links-to-files/

@MartinNowak
Copy link
Member

It seems _d_assocarrayliteralTX could also be affected as it also just uses mutableOf. I'm not sue if you can create a shared AA literal, though.

Don't rely on subtleties, it might become possible at some point and it will be much harder to find this implicit assumptions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants