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

Request for review: Make ty::t type self-sufficient #1759

wants to merge 1 commit into
base: master


None yet
3 participants
Copy link

marijnh commented Feb 5, 2012

This is a rather nice cleanup, but, as described in the commit message, introduces a bit of unsafe code. I may be so blinded by my unwillingness to abandon my pleasant patch that my judgement is clouded. I'd like to have one or two team member sign off on this before I merge it. No need to read the whole patch -- the commit message and mk_t in should give you a good idea of what it does.

(The slowdown seen by the non-unsafe version of this patch has turned into a small speedup in the unsafe version.)

Make ty::t type self-sufficient
It is now no longer needed to have a ty::ctxt to get at the contents
of a ty::t. The straight-forward approach of doing this, simply making
ty::t a box type, unfortunately killed our compiler performance (~15%
slower) through refcounting cost. Thus, this patch now represents
ty::t as an unsafe pointer, assuming that the ty::ctxt, which holds
these boxes alive, outlives any uses of the ty::t values. In the
current compiler this trivially holds, but it is does of course add a
new potential pitfall.

ty::get takes a ty::t and returns a boxed representation of the type.
I've changed calls to ty::struct(X) to do ty::get(X).struct. Type
structs are full of vectors, and copying them every time we wanted to
access them was a bit of a cost.

This comment has been minimized.

Copy link

brson commented Feb 6, 2012

It looks like a win all around to me. Sad that our boxes don't cut it here. Maybe add some comments explaining the devious nature of ty::t.


This comment has been minimized.

Copy link

nikomatsakis commented Feb 6, 2012

I just had some time to look it over. Looks very nice. Also, it's a good way of evaluating the RC overhead.


This comment has been minimized.

Copy link

marijnh commented Feb 6, 2012

Thanks. I'll add some clarifying comments and push it.

@marijnh marijnh closed this Feb 7, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment