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

Add proper parenting to ast_map #26419

Closed
wants to merge 2 commits into from
Closed

Add proper parenting to ast_map #26419

wants to merge 2 commits into from

Conversation

nrc
Copy link
Member

@nrc nrc commented Jun 19, 2015

This adds the parent node in the AST to the ast_map as well as the current, poorly defined parent which is roughly the parent item. Since this just rounds every ast_map entry to 128 bits, it shouldn't use any more memory (because they should be padded to that amount), at least on 64bit systems. The alternative is to only store the parent node and to calculate the parent item. But we need the parent item fairly frequently and I am wary about calculating it on the fly. I think go and use this information for save-analysis.

cc @Manishearth, @rust-lang/compiler

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb
Copy link
Member

eddyb commented Jun 19, 2015

This actually increases the size to 192 bits on x64: the pointer and two NodeIds are 128 bits by themselves.

@nrc
Copy link
Member Author

nrc commented Jun 19, 2015

Ah crap, there is the discriminant, isn't there? I didn't think of that.

@nrc
Copy link
Member Author

nrc commented Jun 19, 2015

I guess I should measure this vs the calculate parent item as we go along version.

@bors
Copy link
Contributor

bors commented Jun 19, 2015

☔ The latest upstream changes (presumably #26351) made this pull request unmergeable. Please resolve the merge conflicts.

@nrc
Copy link
Member Author

nrc commented Jun 29, 2015

I implemented the approach of just storing the immediate parent and calculating the parent item on the fly. It basically has zero impact on compile times which strategy we use (averages over 3 runs):

master: 10m8.3s
storing approach: 10m8.0s
calculating approach: 10m6.8s

For reference, over all runs the min was 10m0.775s and the max 10m13.389s. So the small differences above are basically just noise.

I think I prefer the calculating approach - if speed is equal, then less memory is probably better. It feels slightly better too, although only slightly. I'll submit that as a new PR. Closing this one.

@nrc nrc closed this Jun 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants