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

Remove some unnecessary indirection from AST/HIR structures #30087

Merged
merged 1 commit into from Dec 8, 2015

Conversation

@petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Nov 27, 2015

I've measured the time/memory consumption before and after - the difference is lost in statistical noise, so it's mostly a code simplification.
Sizes of enums are not affected.

r? @nrc

I wonder if AST/HIR visitors could run faster if Ps are systematically removed (except for cases where they control enum sizes). Theoretically they should.
Remaining unnecessary Ps can't be easily removed because many folders accept P<X>s as arguments, but these folders can be converted to accept Xs instead without loss of efficiency.
When I have a mood for some mindless refactoring again, I'll probably try to convert the folders, remove remaining Ps and measure again.

@nrc
Copy link
Member

@nrc nrc commented Nov 28, 2015

The HIR changes seem like a no-brainer - r+. However, I'm wary about changing the AST and breaking a whole lot of code for no real gain - is there any motivation I'm missing or is this just tidying up? If the latter, I'd rather not change the AST.

cc @eddyb who knows about P

@eddyb
Copy link
Member

@eddyb eddyb commented Nov 28, 2015

I should note that some (all?) of these are an artifact of @t and how the ast_map used to work (keeping shared boxes) - now they can disappear.

@petrochenkov
Copy link
Contributor Author

@petrochenkov petrochenkov commented Nov 28, 2015

I'm not sure how much things it will break.
Code reading AST should not be affected (mostly) because P is Deref, quasiquoting should not be affected. Code generating traits, impls, enums directly may be affected.
@Manishearth, maybe you can estimate better?

I'm totally ready to revert the AST part, but I'd like to hear some more opinions, because cleanup is still an improvement, albeit modest.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Nov 29, 2015

I think a bunch of Visitors might break. No biggie.

Clippy would break, because we use P in some of our functions (which expect the corresponding fields to be P<T>), but it's simple enough to upgrade over.

Code using aster on nightly will probably break too (so, serde). Not sure how easy the upgrade is there.

I'd expect some speed improvements here, surprised there aren't any. But I'm overall okay with this breakage.

@bors
Copy link
Contributor

@bors bors commented Dec 4, 2015

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

@petrochenkov petrochenkov force-pushed the petrochenkov:indi branch from 47da574 to 951cee4 Dec 4, 2015
@petrochenkov
Copy link
Contributor Author

@petrochenkov petrochenkov commented Dec 4, 2015

Rebased.

@nrc
Copy link
Member

@nrc nrc commented Dec 6, 2015

It seems to me that @Manishearth's list of bustage is enough to justify not landing the AST parts.

I'd like to land the HIR parts now but not the AST parts. I do think the AST changes are worthwhile though, and if there is a PR in the future which makes breaking changes to the AST, then perhaps this could be landed at the same time.

@petrochenkov
Copy link
Contributor Author

@petrochenkov petrochenkov commented Dec 6, 2015

if there is a PR in the future which makes breaking changes to the AST, then perhaps this could be landed at the same time.

There is such a PR! #29850 landed two days ago. (It is the reason why I had to rebase this PR as well)

@nrc
Copy link
Member

@nrc nrc commented Dec 6, 2015

Ah, its a shame we didn't land them together. I think they have to land together (or at least to hit the same nightly) to get the benefit.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Dec 7, 2015

Note that I'm mostly okay with clippy breaking on minor things like this if I know in advance, and can fix it up pretty much immediately.

The existence of syntex makes things hard for us, though, since tiny public API changes still break compat when aster/etc are in the picture. Not sure how to handle that.

@petrochenkov petrochenkov force-pushed the petrochenkov:indi branch from 951cee4 to ca88e9c Dec 7, 2015
@petrochenkov
Copy link
Contributor Author

@petrochenkov petrochenkov commented Dec 7, 2015

Updated with AST changes reverted.
AST changes still live in a separate branch https://github.com/petrochenkov/rust/tree/indi_ast in case someone wants to apply them when the appropriate time comes.

@nrc
Copy link
Member

@nrc nrc commented Dec 7, 2015

@bors: r+

@bors
Copy link
Contributor

@bors bors commented Dec 7, 2015

📌 Commit ca88e9c has been approved by nrc

@bors
Copy link
Contributor

@bors bors commented Dec 7, 2015

Testing commit ca88e9c with merge acf4e0b...

bors added a commit that referenced this pull request Dec 7, 2015
I've measured the time/memory consumption before and after - the difference is lost in statistical noise, so it's mostly a code simplification.
Sizes of `enum`s are not affected.

r? @nrc

I wonder if AST/HIR visitors could run faster if `P`s are systematically removed (except for cases where they control `enum` sizes). Theoretically they should.
Remaining unnecessary `P`s can't be easily removed because many folders accept `P<X>`s as arguments, but these folders can be converted to accept `X`s instead without loss of efficiency.
When I have a mood for some mindless refactoring again, I'll probably try to convert the folders, remove remaining `P`s and measure again.
@bors bors merged commit ca88e9c into rust-lang:master Dec 8, 2015
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details
bors added a commit that referenced this pull request Feb 12, 2016
@petrochenkov petrochenkov deleted the petrochenkov:indi branch Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants