use old ctx if has same expand environment during decode span by bvanjoi · Pull Request #127279 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

bvanjoi

Fixes #112680

The root reason why #112680 failed with incremental compilation on the second attempt is the difference in opaque between the span of the field ident and the span in the incremental cache at tcx.def_ident_span(field.did).

span_a_ctx -> SyntaxContextData { opaque: span_a_ctx, opaque_and_semitransparent: span_a_ctx, // .... }

span_b_ctx -> SyntaxContextData { opaque: span_b_ctx, // note span_b_ctx is not same as span_a_ctx opaque_and_semitransparent: span_b_ctx, // .... }

Although they have the same parent (both refer to the root) and outer_expn, I cannot find the specific connection between them. Therefore, I chose a solution that may not be the best: give up the incremental compile cache to ensure we can use span_a in this case.

r? @petrochenkov Do you have any advice on this? Or perhaps this solution is acceptable?

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Jul 3, 2024

@petrochenkov

Could you check if the issue reproduces with #119412?
ident in macros from other crates is known to have buggy spans, but I don't know whether this issue is related to that or not.

@bvanjoi

Could you check if the issue reproduces with #119412

Unfortunately, this issue still exists

@cjgillot

Whether def_ident_span is cached on disk or not must not change the returned value. If it does, the bug is in cache decoding. Avoiding the cache will just hide the bug until another code path hits it.

@bvanjoi

@bvanjoi

Whether def_ident_span is cached on disk or not must not change the returned value

Yep, but I think this might be an omission point during decoding spans that come from the disk cache: sometimes it may not be necessary to generate a new ctxt, and using the old one is enough. So in the latest PR, I added this condition: if it comes from the same macro expand environment and the old ctxt exists, then use the old one.

This means the span_b will become:

span_b_ctx -> SyntaxContextData { opaque: span_a.opaque, opaque_and_semitransparent: span_a.opaque_and_semitransparent, // .... }

I'm not sure if it's completely correct, but it's more convincing than simply disabling the disk cache for ident spans.

@bvanjoi bvanjoi changed the titlenot cache def_ident_span from disk use old ctx if has same expand environment during decode span

Jul 4, 2024

@petrochenkov

I think the new fix is in the right direction.

In SyntaxContextData these three fields are substantial

outer_expn: ExpnId,
outer_transparency: Transparency,
parent: SyntaxContext,

and these two fields are caches / precomputed values for some operations on the substantial fields

/// This context, but with all transparent and semi-transparent expansions filtered away.
opaque: SyntaxContext,
/// This context, but with all transparent expansions filtered away.
opaque_and_semitransparent: SyntaxContext,

The last field seems to also be ignored during decoding (with a reasonable explanation).

/// Name of the crate to which `$crate` with this context would resolve.
dollar_crate_name: Symbol,

So there are two possible strategies for encoding/decoding SyntaxContextData.

@bvanjoi

why they don't match

As you can see, the fields outer_expn, outer_transparency, and parent have not changed, so we don't need to consider them. I'd like to explain why opaque is not equal between the first compilation and the second with incremental compilation.

The content of span which need to encode is:

span_a_ctx -> SyntaxContextData { opaque: span_a_ctx, //~^ note that span_a_data.opaque and span_a_ctx have the same value // .... }

And during the second compilation:

@Dylan-DPC

@bvanjoi

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Aug 21, 2024

@petrochenkov

@bors

📌 Commit 07481b9 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Aug 21, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Aug 21, 2024

@bors

…iaskrgr

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Aug 21, 2024

@bors

…iaskrgr

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

Aug 22, 2024

@jieyouxu

bors added a commit to rust-lang-ci/rust that referenced this pull request

Aug 22, 2024

@bors

…iaskrgr

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Aug 22, 2024

@rust-timer

@pnkfelix

As a heads up, this seems to have been responsible for the (minor) incr-patched regressions reported on rollup PR #129365, as you can see from #129365 (comment)

Having said that, the degree of the regression here is minor, and the fix here is patching over a legitimate problem. So I am not going to actually mark this with the perf regression label.

Nonetheless It might be worth still investigating the potential deeper cause of the problem being patched over, as described by @petrochenkov above. If we were to do that, we might be able to revert this PR. (Who knows what end effect all that will have on performance; its unknown, so I'm not going to claim it will recover the minor performance regression flagged above...)

bors added a commit to rust-lang-ci/rust that referenced this pull request

Aug 30, 2024

@bors

…xt, r=

dont clone old syntax context

I guess this regression was caused by too many clones, so this is an attempt to use the old value rather than cloning it. Perhaps a better approach would be to ensure that only the substantial fields mentioned in this [comment](rust-lang#127279 (comment)) are cacheable.

Anyway, let's run a perf test to see if this can solve the problem.

r? @pnkfelix or @petrochenkov

bors added a commit to rust-lang-ci/rust that referenced this pull request

Aug 31, 2024

@bors

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 11, 2024

@bors

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 29, 2025

@bors

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.