Skip to content
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

RFC: IdDict{K,V} as wrapped ObjectIdDict #25196

Closed

Conversation

mauro3
Copy link
Contributor

@mauro3 mauro3 commented Dec 19, 2017

This adds an IdDict{K,V} which uses === and object_id for equality and hashing as is needed for #24354. It is implemented by wrapping the good old ObjectIdDict and using type assertions on the getters/setters. This is an alternate implementation of #24932, as was suggested by @JeffBezanson in #24932 (comment). Unlike #24932, this PR passes all tests locally and should be mostly ready to go.

This PR deprecates the "old" ObjectIdDict in favor of the new IdDict. To make this work I had to rename ObjectIdDict to something else: _ObjectIdDict. Note though that _ObjectIdDict is still used and needed in the core of Julia.

Performance of the IdDict is a bit worse (25%) than for ObjectIdDict, presumably because of the extra layer, as the somewhat rudimentary tests in the gist show.

RFCs:

  • is the rename of ObjectIdDict necessary or could it be deprecated without rename?
  • One thing I am not sure about is the, now updated, following statement in the docs: "Alternatively, you can use the IdDict dictionary type, which is specially handled by precompilation so that it is safe to initialize at compile-time." Is this still true? If not it probably should be made true. This needs checking by someone!

This should probably be squashed before merging.

@ararslan ararslan added the collections Data structures holding multiple items, e.g. sets label Dec 19, 2017
@JeffBezanson
Copy link
Member

It seems to me _ObjectIdDict should be unnecessary. What goes wrong without it?

@mauro3
Copy link
Contributor Author

mauro3 commented Dec 20, 2017

Not renaming (going back to 7d4908e) and adding to deprecated.jl:

export ObjectIdDict
const _dep_message_ObjectIdDict = ", use IdDict{Any, Any} instead"
deprecate(Base, :ObjectIdDict)

compiles but throws dep-warns from the (legitimate) usages in Base. Can you point me to how you would go about it? One option could be to only have ObjectIdDict in Core, but I don't know how that works or whether that is a good strategy. So, I need a bit of guidance for this.

@JeffBezanson
Copy link
Member

The uses in Base can use IdDict{Any,Any} directly, or we could provide the ObjectIdDict alias without deprecation.

@mauro3
Copy link
Contributor Author

mauro3 commented Dec 20, 2017

I changed the usages of ObjectIdDict which are after dict.jl in the boot-strap to IdDict, but the ones before (in Core) still use ObjectIdDict. To move the Core-uses to IdDict is beyond my skills.

I think it would be nice to deprecate ObjectIdDict, otherwise we provide two types with almost the same use, leading to function signatures like f(d::Union{IdDict,ObjectIdDict}) = .... One strategy could be to rename & deprecate in 0.7 and in 1.0 rename again to ObjectIdDict but not export anymore (to get rid of the ugly _ObjectIdDict name).

@JeffBezanson
Copy link
Member

Replaced by #25210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants