Make fields of `Span` private by petrochenkov · Pull Request #43968 · rust-lang/rust

petrochenkov · 2017-08-18T00:11:18Z

I actually tried to intern spans and benchmark the result^*, and this was a prerequisite.
This kind of encapsulation will be a prerequisite for any other attempt to compress span's representation, so I decided to submit this change alone.

The issue #43088 seems relevant, but it looks like SpanId won't be able to reuse this interface, unless the tables are global (like interner that I tried) and are not a part of HIR.
r? @michaelwoerister anyway

^* Interning means 2-3 times more space is required for a single span, but duplicates are free. In practice it turned out that duplicates are not that common, so more memory was wasted by interning rather than saved.

petrochenkov · 2017-08-18T00:12:58Z

I'm not sure set_x are the best possible names since the functions are not mutating, but at least I guarded them from mistakes with #[must_use].

Should the fields be named with_x? In my mind that conjures the fact that the method returns a copy of the same type with the field having the new value.

Yeah, seems better, I'll rename.

kennytm · 2017-08-18T01:24:00Z

Huh? Just return span.set_ctxt(SyntaxContext::empty().apply_mark(mark)) directly.

estebank · 2017-08-18T06:23:01Z

Wouldn't it make more sense to do the following in all the set_x methods?
Span{lo, ..self}

It was necessary with interning (the same is true for #43968 (comment)). It will be necessary with any non-trivial Span::new/Span::lo/etc, so I left it.

estebank · 2017-08-18T06:23:06Z

Should the fields be named with_x? In my mind that conjures the fact that the method returns a copy of the same type with the field having the new value.

estebank · 2017-08-18T06:24:23Z

Is it really necessary to use the public getters in the impl?

I guess it would be if spans were really interned. For now I'd also opt for just accessing the fields directly.

michaelwoerister

Looks good to me. Thanks, @petrochenkov! I'm also for renaming the set_x methods to with_x. Other than that (and maybe not using the getters in impl Span) this is good to go.

You might be interested in this: A couple of years ago I did some experiments with span interning plus storing the span information directly in the interning key, if it fit in there (like a tagged pointer). The results were actually quite promising, if I remember correctly: https://internals.rust-lang.org/t/rfc-compiler-refactoring-spans/1357/23. Maybe you want to revisit interning with this additional optimization?

michaelwoerister · 2017-08-18T09:12:34Z

I guess it would be if spans were really interned. For now I'd also opt for just accessing the fields directly.

petrochenkov · 2017-08-18T10:04:04Z

@michaelwoerister

The results were actually quite promising, if I remember correctly: https://internals.rust-lang.org/t/rfc-compiler-refactoring-spans/1357/23. Maybe you want to revisit interning with this additional optimization?

I remembered there was a thread about this somewhere! Thanks for the link.
Yes, I was going to experiment further with keeping "short" spans inline and other spans "somewhere else".

petrochenkov · 2017-08-18T22:27:23Z

Updated.

estebank · 2017-08-19T03:06:20Z

I believe this can be replaced with Some(sp_lhs.to(sp_rhs)).

bors · 2017-08-19T05:06:57Z

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

michaelwoerister · 2017-08-19T09:31:28Z

Thanks! r=me after rebasing.

petrochenkov · 2017-08-19T12:37:48Z

@bors r=michaelwoerister

bors · 2017-08-19T12:37:49Z

📌 Commit cdae234 has been approved by michaelwoerister

bors · 2017-08-19T16:28:53Z

⌛ Testing commit cdae2343dba5a47c3e9846b6e7c8521ac1102c2c with merge 17e924eb9a0484958fe948c38b20c93644eb3137...

bors · 2017-08-19T17:34:34Z

💔 Test failed - status-travis

petrochenkov · 2017-08-19T17:39:13Z

Sigh, need to send a PR to rustfmt first.

kennytm · 2017-08-20T08:45:26Z

We've hit a deadlock situation where the rustfmt PR cannot pass without the get/set methods added in this PR, and this PR cannot pass without rustfmt stop reading the now-private fields. I don't think turning rustfmt's status red is a good idea.

I think this PR should be split into two stages,

This PR keep the fields public, so rustfmt doesn't need to be updated at the moment.
rustfmt (as well as clippy, rls, etc.) all accept PRs stop using the fields.
Submit another PR to turn the fields private (can be done simultaneously with step 2).

petrochenkov · 2017-08-20T09:27:52Z

@kennytm
Situations like this were previously resolved by creating a new branch in rustfmt/rls, merging the breaking patch into that branch and not master, then setting rls submodule in rust-lang/rust to that branch.

petrochenkov · 2017-08-20T20:07:13Z

I gathered some span statistics from rustc/libstd and found a significant number of "reverse" spans with lo > hi!
That seems bad, some code may work correctly with such spans, but I've certainly seen ICEs caused by code that doesn't work with them.

I'm going to add "normalization" to lo <= hi into Span::new.
It also boosts the percent of inline spans a bit when compression is used.

to land rust-lang/rust#43968

petrochenkov · 2017-08-20T23:02:47Z

@bors r=michaelwoerister

bors · 2017-08-20T23:02:47Z

📌 Commit 3da163f has been approved by michaelwoerister

bors · 2017-08-20T23:02:57Z

⌛ Testing commit 3da163f55b1dd62f247f63edee30f64e3e7da2f5 with merge 4ad2c420bbe6368916158ddb4b354a7652a02d72...

bors · 2017-08-21T00:50:08Z

💔 Test failed - status-travis

petrochenkov · 2017-08-21T10:07:43Z

@bors r=michaelwoerister

bors · 2017-08-21T10:07:44Z

📌 Commit c4125e2 has been approved by michaelwoerister

bors · 2017-08-21T10:52:17Z

⌛ Testing commit c4125e26389fbe5beceed0c562a47dfb3e45df07 with merge c1ad7551a567319ae597b9643a3dcf627d9bf8e2...

bors · 2017-08-21T12:46:09Z

💔 Test failed - status-travis

kennytm · 2017-08-21T13:02:33Z

x86_64-gnu-aux, same error. It's legit then.

nrc · 2017-08-22T00:41:34Z

RLS tests timed out or some span is not found. Maybe legit

Possibly a bad span produced in save-analysis?

petrochenkov · 2017-08-22T19:13:21Z

The errors are caused by RLS update, not by span patches, i.e. 9061581 alone (without rust-lang/rls@f4e6f16) is enough to reproduce them.
In theory, #44028 should hit them as well.
cc @nrc

estebank · 2017-08-22T21:20:23Z

Could you provide a dump of the reversed spans? I've seen them every now and then but have put off investigating the root cause(s). There are a few possible very unseemly results that could happen, for example when doing end.to(start), so I'd hate to just mask the incorrect spans by doing the obvious fix but rather fix the source (probably during parsing).

petrochenkov · 2017-08-23T21:26:04Z

Waiting on #44028

petrochenkov · 2017-08-23T21:32:16Z

@estebank

Could you provide a dump of the reversed spans?

I have only numbers unfortunately. Looks like the only way to obtain the source of inverted spans from Span::new is backtrace.
Once this PR lands, Span::new will be the single point of span creation, so you will be able to detect inverted spans by inserting an assert(lo <= hi) into it and building everything with RUST_BACKTRACE=1.
EDIT: Maybe that assert could be left there forever once all inverted spans are fixed.

estebank · 2017-08-24T00:43:54Z

Sounds reasonable. Will do. Are asserts only enabled on nightly? Would rather not cause ICEs for a rather benign bug just because it isn't caught in the test, but would like to see he effects of it on cargo bomb.

bors · 2017-08-26T04:09:09Z

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

petrochenkov · 2017-08-26T09:23:10Z

@estebank

Are asserts only enabled on nightly?

Not sure about debug_assert, but assert is always enabled.
FWIW, nightly can be detected through an environmental variable CFG_RELEASE_CHANNEL, but I'm not sure it's a good idea.

to land rust-lang/rust#43968

This helps to avoid landing changes to rustc and rustfmt in one step

petrochenkov · 2017-08-29T22:43:37Z

I'll try to land changes to rustc and rustfmt in two steps (rustfmt is moving forward too fast), so I made fields of Span public again for compatibility.
@bors r=michaelwoerister

bors · 2017-08-29T22:43:37Z

📌 Commit a0c3264 has been approved by michaelwoerister

bors · 2017-08-30T11:08:32Z

⌛ Testing commit a0c3264 with merge ca9cf35...

@michaelwoerister

Make fields of `Span` private I actually tried to intern spans and benchmark the result<sup>*</sup>, and this was a prerequisite. This kind of encapsulation will be a prerequisite for any other attempt to compress span's representation, so I decided to submit this change alone. The issue #43088 seems relevant, but it looks like `SpanId` won't be able to reuse this interface, unless the tables are global (like interner that I tried) and are not a part of HIR. r? @michaelwoerister anyway <sup>*</sup> Interning means 2-3 times more space is required for a single span, but duplicates are free. In practice it turned out that duplicates are not *that* common, so more memory was wasted by interning rather than saved.

bors · 2017-08-30T13:56:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing ca9cf35 to master...

rust-highfive assigned michaelwoerister Aug 18, 2017

petrochenkov commented Aug 18, 2017
View reviewed changes

kennytm reviewed Aug 18, 2017
View reviewed changes

estebank reviewed Aug 18, 2017
View reviewed changes

michaelwoerister reviewed Aug 18, 2017
View reviewed changes

petrochenkov force-pushed the span2 branch from 2c1d664 to 9fed6daCompare August 18, 2017 22:26

estebank reviewed Aug 19, 2017
View reviewed changes

Comment threadsrc/libsyntax/codemap.rs Outdated
estebankAug 19, 2017
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be replaced with Some(sp_lhs.to(sp_rhs)).

mcarton mentioned this pull request Aug 19, 2017
Breakage incoming rust-lang/rust-clippy#1972
Closed

petrochenkov force-pushed the span2 branch from 9fed6da to cdae234Compare August 19, 2017 12:37

petrochenkov mentioned this pull request Aug 19, 2017
Use getters to access Span fields rust-lang/rustfmt#1899
Closed

nrc pushed a commit to rust-lang/rls that referenced this pull request Aug 20, 2017
Depend on the span branch of rustfmt
f4e6f16
to land rust-lang/rust#43968

petrochenkov force-pushed the span2 branch from cdae234 to 3da163fCompare August 20, 2017 23:02

nrc mentioned this pull request Aug 22, 2017
Documents managing tools in the Rust repo rust-dev-tools/dev-tools-team#27
Merged

arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 22, 2017

nrc pushed a commit to rust-lang/rls that referenced this pull request Aug 27, 2017
Depend on the span branch of rustfmt
903cb9f
to land rust-lang/rust#43968

petrochenkov added 3 commits August 30, 2017 01:38

Make fields of Span private
3da868d

Normalize order of lo and hi in Span::new
71dfe64

Make fields of Span public again
a0c3264
This helps to avoid landing changes to rustc and rustfmt in one step

petrochenkov force-pushed the span2 branch from c4125e2 to a0c3264Compare August 29, 2017 22:41

bors merged commit a0c3264 into rust-lang:masterAug 30, 2017

This was referenced Aug 30, 2017
Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171
Merged
Add expansion info to crate metadata #43847
Closed

petrochenkov deleted the span2 branch September 10, 2017 22:00

Uh oh!

Uh oh!

Conversation

petrochenkov commented Aug 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkovAug 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelwoerister left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Aug 18, 2017

Uh oh!

petrochenkov commented Aug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Aug 19, 2017

Uh oh!

michaelwoerister commented Aug 19, 2017

Uh oh!

petrochenkov commented Aug 19, 2017

Uh oh!

bors commented Aug 19, 2017

Uh oh!

bors commented Aug 19, 2017

Uh oh!

bors commented Aug 19, 2017

Uh oh!

petrochenkov commented Aug 19, 2017

Uh oh!

kennytm commented Aug 20, 2017

Uh oh!

petrochenkov commented Aug 20, 2017

Uh oh!

petrochenkov commented Aug 20, 2017

Uh oh!

petrochenkov commented Aug 20, 2017

Uh oh!

bors commented Aug 20, 2017

Uh oh!

bors commented Aug 20, 2017

Uh oh!

bors commented Aug 21, 2017

Uh oh!

petrochenkov commented Aug 21, 2017

Uh oh!

bors commented Aug 21, 2017

Uh oh!

bors commented Aug 21, 2017

Uh oh!

bors commented Aug 21, 2017

Uh oh!

kennytm commented Aug 21, 2017

Uh oh!

nrc commented Aug 22, 2017

Uh oh!

petrochenkov commented Aug 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Aug 18, 2017•
edited
Loading

petrochenkovAug 18, 2017•
edited
Loading

petrochenkov commented Aug 22, 2017•
edited
Loading

petrochenkov commented Aug 23, 2017•
edited
Loading

petrochenkov commented Aug 26, 2017•
edited
Loading