Add a MutVisitor for the AST #57662

Zoxc · 2019-01-16T07:23:15Z

No description provided.

rust-highfive · 2019-01-16T07:23:25Z

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

rust-highfive · 2019-01-16T07:30:45Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:2db3b706:start=1547623480279563046,finish=1547623481241714349,duration=962151303
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:03:29] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:29] tidy error: /checkout/src/libsyntax/ast.rs:2092: line longer than 100 chars
[00:03:29] tidy error: /checkout/src/libsyntax/visit_mut.rs:42: trailing whitespace
[00:03:29] tidy error: /checkout/src/libsyntax/visit_mut.rs:500: line longer than 100 chars
[00:03:29] tidy error: /checkout/src/libsyntax/visit_mut.rs:738: line longer than 100 chars
[00:03:29] tidy error: /checkout/src/libsyntax/visit_mut.rs:752: line longer than 100 chars
[00:03:29] tidy error: /checkout/src/libsyntax/visit_mut.rs:758: line longer than 100 chars
[00:03:29] tidy error: /checkout/src/libsyntax/visit_mut.rs:764: line longer than 100 chars
[00:03:31] some tidy checks failed
[00:03:31] 
[00:03:31] 
[00:03:31] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:31] 
[00:03:31] 
[00:03:31] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:31] Build completed unsuccessfully in 0:00:46
[00:03:31] Build completed unsuccessfully in 0:00:46
[00:03:31] make: *** [tidy] Error 1
[00:03:31] Makefile:69: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0ae948f4
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Jan 16 07:28:23 UTC 2019
---
travis_time:end:0a9b0cc2:start=1547623704375112740,finish=1547623704379824881,duration=4712141
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:06d18916
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:071e2786
travis_time:start:071e2786
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:2b1fdee1
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

petrochenkov · 2019-01-16T15:08:24Z

@Zoxc
Any specific reason to expect this to be faster than folding?

From what I understand the difference is mostly "noop visiting" vs "noop folding", and both should be pretty cheap.
I can imagine noop folding doing too much memcpy-ing things around, though, when it's not inlined.

This can probably be measured by a targeted benchmark - "noop fold the crate N times" vs "noop visit the crate N times".

src/librustc_allocator/expand.rs

petrochenkov · 2019-01-16T15:21:52Z

src/libsyntax/visit_mut.rs

+            }
+
+            if let Some(replacement) = $visitor.$method(&mut $list[i], $(, $extra_args)*) {
+                $list.remove(i);


The case replacement.len() == 1 can be optimized to assignment.

petrochenkov · 2019-01-16T15:22:33Z

src/libsyntax/visit_mut.rs

+                $list.remove(i);
+                let rlen = replacement.len();
+                for (j, r) in replacement.into_iter().enumerate() {
+                    $list.insert(i + j, r);


Also, don't we have a range insert, like list.insert(replacement.into_iter()) or something?

Hmm, looks like we don't.
Vec::splice looks very similar though.

Zoxc · 2019-01-17T04:59:09Z

@petrochenkov This does give a ~3x speedup to ExpandAllocatorDirectives.

rust-highfive · 2019-01-17T05:35:49Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:032b207e:start=1547702093936394375,finish=1547702096142544476,duration=2206150101
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:18:53]    Compiling rustc_allocator v0.0.0 (/checkout/src/librustc_allocator)
[00:18:53] error: unused import: `Mac`
[00:18:53]  --> src/librustc_allocator/expand.rs:6:9
[00:18:53]   |
[00:18:53] 6 |         Mac, Mod, Mutability, Ty, TyKind, Unsafety, VisibilityKind, NodeId,
[00:18:53]   |
[00:18:53]   = note: `-D unused-imports` implied by `-D warnings`
[00:18:53] 
[00:18:53] error: aborting due to previous error
---
[00:20:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:20:36] expected success, got: exit code: 101
[00:20:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:20:36] Build completed unsuccessfully in 0:17:05
[00:20:36] make: *** [all] Error 1
[00:20:36] Makefile:18: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:001b03ca
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu Jan 17 05:35:43 UTC 2019
---
travis_time:end:03e54fb0:start=1547703344471008513,finish=1547703344476549427,duration=5540914
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:2b04016a
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:10478575
travis_time:start:10478575
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:3019462a
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nikomatsakis · 2019-01-24T22:37:03Z

r? @petrochenkov -- I trust them to handle this one =)

petrochenkov · 2019-01-29T23:51:07Z

@Zoxc
There's one thing that can be done with a folder, but not with a visitor with the current lifetime setup.
AST fragments in token streams in macros/attributes are not tied to the usual AST lifetime, for this reason you can't e.g. call visit_item on an item inside of an interpolated token NtItem.
Folders are not tied to the AST lifetime so much, so they can do things like fn noop_fold_interpolated.

However, I'm not sure folding interpolated tokens is even necessary now (at least visitors ignore them entirely, but I don't remember this causing issues), but I haven't checked in more details yet.

Zoxc · 2019-01-30T00:00:59Z

I don't see why you couldn't do that if that is desirable, there's no "AST lifetime" for this visitor.

petrochenkov · 2019-01-30T10:23:53Z

Ah, I see, MutVisitor doesn't have the 'ast lifetime.
(I don't actually know why the old Visitor needs it.)

Do you plan to convert more folders before merging?
Generally, I'd want to convert everything and can continue this work myself, but probably not before mid-February.

Zoxc · 2019-01-30T17:28:51Z

I'm going to apply this to StripUnconfigured too

Zoxc · 2019-01-30T23:56:07Z

@bors try

bors · 2019-01-30T23:56:20Z

⌛ Trying commit a9ff07e with merge a0eec98...

Add a MutVisitor for the AST

bors · 2019-01-31T02:08:16Z

☀️ Test successful - checks-travis
State: approved= try=True

Zoxc · 2019-01-31T02:19:31Z

@rust-timer build a0eec98

rust-timer · 2019-01-31T02:19:32Z

Success: Queued a0eec98 with parent 147311c, comparison URL.

rust-timer · 2019-01-31T05:03:21Z

Finished benchmarking try commit a0eec98

ishitatsuyuki · 2019-01-31T09:07:02Z

Perf summary: mostly noise, syn-opt spurious regression, ucd improvement on incremental by 1%.

petrochenkov · 2019-02-01T08:40:13Z

@Zoxc
Could you remove the likely!s? It's not something that should be used casually.

Could you also make the synthetic benchmark "run noop fold the crate N times" vs "run noop mut visit the crate N times" and tell the numbers?

Beside that, converting other folders may require making some extensions to MutVisitor, but that's ok and can be addressed later (fixing some unnecessary copy-paste-ness in process as well).

petrochenkov · 2019-02-01T08:46:09Z

cc @rust-lang/compiler
Do you have any comments regarding use of MutVisitor instead of Folder for modifying AST in general?

This brings some modest performance improvements for the total time (and not so modest for some specific passes), but the code may be a bit uglier perhaps (or perhaps not, when all folders are converted).

I'll r+ in a few days if there are no concerns.

petrochenkov · 2019-02-02T08:40:42Z

An alternative PR appeared - #58061.

eddyb · 2019-02-02T10:51:46Z

IMO we should stop folding/mutating ASTs completely and have visitors and lowering to HIR use separate replacement maps.

This way we can make progress towards incremental macro expansion.

petrochenkov · 2019-02-03T11:41:39Z

@eddyb

IMO we should stop folding ASTs completely and have visitors

I'm not sure whether you approve what this PR does or have some other kind of visitors in mind here...

lowering to HIR have separate replacement map

... and it's also not clear to me what the "replacement map" is.

eddyb · 2019-02-03T12:59:48Z

I meant immutable visitors, to be clear. In other worse, make the AST more like a persistent data structure.

As for the "replacement map" - it already exists, for macro expansion (mapping some "placeholder" IDs to AST fragments).
We can generalize that to record all changes we want to make to the AST, as replacing a node (referenced by e.g. ID assigned at parse time) with another (or a list of nodes, in the case of e.g. items)

petrochenkov · 2019-02-03T18:16:13Z

Ok, MutVisitor is certainly closer to that state than existing Folder.

petrochenkov · 2019-02-04T11:58:17Z

Blocked on #58061.
(#58061 (comment))

nnethercote · 2019-02-06T01:09:13Z

@Zoxc: I apologize for the overlap with #58061. I'm sorry to have caused you wasted time.

I am contemplating the same change for TypeFolder/TypeFoldable. It shows up a lot more in profiles than fold::Folder did, so the potential for speed-ups seems good. But I have some unrelated work I need to do, so I might not get to it for a while. If you are interested I'm happy for you to do it instead.

petrochenkov · 2019-02-08T05:56:37Z

@Zoxc
I was just going to suggest closing for now.
I'm going to do some work on the expansion visitor in the near future, then collect some statistics for "1 -> many" transformations, and then attempt to make tweaks based on that statistics.

nnethercote · 2019-02-13T21:30:03Z

I am contemplating the same change for TypeFolder/TypeFoldable. It shows up a lot more in profiles than fold::Folder did, so the potential for speed-ups seems good.

@Zoxc: I looked into this a bit. I don't think it will be able to benefit from the same change, due mostly to the heavy use of interning. E.g. consider this function, which is one of the hottest folders:

rust/src/librustc/ty/subst.rs

Lines 327 to 337 in e544947

    
           fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, folder: &mut F) -> Self { 
        
               let params: SmallVec<[_; 8]> = self.iter().map(|k| k.fold_with(folder)).collect(); 
        
               // If folding doesn't change the substs, it's faster to avoid 
        
               // calling `mk_substs` and instead reuse the existing substs. 
        
               if params[..] == self[..] { 
        
                   self 
        
               } else { 
        
                   folder.tcx().intern_substs(&params) 
        
               } 
        
           }

It's folding a &'tcx Substs<'tcx>. Even if the &self is changed to self, there's a problem, which is that we can't mutate a &'tcx Substs<'tcx>. Mutating interned values just doesn't work.

eddyb · 2019-02-14T11:48:53Z

We should be moving more to interning and persistence, even of AST fragments, because of the benefits it has for being able to do anything incremental.

That's why I think MutVisitor is going in the wrong direction, and Folder should go away too.
We've done this for the HIR already, now it's time for the AST to become immutable.

rust-highfive assigned nikomatsakis Jan 16, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2019

petrochenkov self-assigned this Jan 16, 2019

petrochenkov reviewed Jan 16, 2019

View reviewed changes

src/librustc_allocator/expand.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Jan 16, 2019

View reviewed changes

src/librustc_allocator/expand.rs Show resolved Hide resolved

petrochenkov reviewed Jan 16, 2019

View reviewed changes

Zoxc force-pushed the ast-mut-visitor branch from a957de8 to 00c9fa1 Compare January 17, 2019 05:13

Zoxc force-pushed the ast-mut-visitor branch from 00c9fa1 to 384c60c Compare January 17, 2019 06:32

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2019

rust-highfive unassigned nikomatsakis Jan 24, 2019

Zoxc force-pushed the ast-mut-visitor branch from 384c60c to eada91d Compare January 27, 2019 18:55

Zoxc changed the title ~~[WIP] Add a MutVisitor for the AST~~ Add a MutVisitor for the AST Jan 28, 2019

Zoxc added 2 commits January 30, 2019 15:05

Add a MutVisitor for the AST, and use it for ExpandAllocatorDirectives

06f0c62

Add an Action enum to represent how we want to change the AST

f2060b6

Convert StripUnconfigured to MutVisitor

a9ff07e

Zoxc force-pushed the ast-mut-visitor branch from eada91d to a9ff07e Compare January 30, 2019 18:23

bors added a commit that referenced this pull request Jan 30, 2019

Auto merge of #57662 - Zoxc:ast-mut-visitor, r=<try>

a0eec98

Add a MutVisitor for the AST

petrochenkov mentioned this pull request Feb 1, 2019

Overhaul syntax::fold::Folder. #58061

Merged

petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2019

Zoxc closed this Feb 8, 2019

petrochenkov mentioned this pull request Feb 15, 2019

[DO NOT MERGE] syntax: try ref-counting the AST. #58482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a MutVisitor for the AST #57662

Add a MutVisitor for the AST #57662

Zoxc commented Jan 16, 2019

rust-highfive commented Jan 16, 2019

rust-highfive commented Jan 16, 2019

petrochenkov commented Jan 16, 2019 •

edited

Loading

petrochenkov Jan 16, 2019

petrochenkov Jan 16, 2019

petrochenkov Jan 16, 2019

Zoxc commented Jan 17, 2019

rust-highfive commented Jan 17, 2019

nikomatsakis commented Jan 24, 2019

petrochenkov commented Jan 29, 2019

Zoxc commented Jan 30, 2019

petrochenkov commented Jan 30, 2019

Zoxc commented Jan 30, 2019

Zoxc commented Jan 30, 2019

bors commented Jan 30, 2019

bors commented Jan 31, 2019

Zoxc commented Jan 31, 2019

rust-timer commented Jan 31, 2019

rust-timer commented Jan 31, 2019

ishitatsuyuki commented Jan 31, 2019

petrochenkov commented Feb 1, 2019 •

edited

Loading

petrochenkov commented Feb 1, 2019

petrochenkov commented Feb 2, 2019

eddyb commented Feb 2, 2019 •

edited

Loading

petrochenkov commented Feb 3, 2019

eddyb commented Feb 3, 2019

petrochenkov commented Feb 3, 2019

petrochenkov commented Feb 4, 2019

nnethercote commented Feb 6, 2019

petrochenkov commented Feb 8, 2019

nnethercote commented Feb 13, 2019

eddyb commented Feb 14, 2019 •

edited

Loading

Add a MutVisitor for the AST #57662

Add a MutVisitor for the AST #57662

Conversation

Zoxc commented Jan 16, 2019

rust-highfive commented Jan 16, 2019

rust-highfive commented Jan 16, 2019

petrochenkov commented Jan 16, 2019 • edited Loading

petrochenkov Jan 16, 2019

Choose a reason for hiding this comment

petrochenkov Jan 16, 2019

Choose a reason for hiding this comment

petrochenkov Jan 16, 2019

Choose a reason for hiding this comment

Zoxc commented Jan 17, 2019

rust-highfive commented Jan 17, 2019

nikomatsakis commented Jan 24, 2019

petrochenkov commented Jan 29, 2019

Zoxc commented Jan 30, 2019

petrochenkov commented Jan 30, 2019

Zoxc commented Jan 30, 2019

Zoxc commented Jan 30, 2019

bors commented Jan 30, 2019

bors commented Jan 31, 2019

Zoxc commented Jan 31, 2019

rust-timer commented Jan 31, 2019

rust-timer commented Jan 31, 2019

ishitatsuyuki commented Jan 31, 2019

petrochenkov commented Feb 1, 2019 • edited Loading

petrochenkov commented Feb 1, 2019

petrochenkov commented Feb 2, 2019

eddyb commented Feb 2, 2019 • edited Loading

petrochenkov commented Feb 3, 2019

eddyb commented Feb 3, 2019

petrochenkov commented Feb 3, 2019

petrochenkov commented Feb 4, 2019

nnethercote commented Feb 6, 2019

petrochenkov commented Feb 8, 2019

nnethercote commented Feb 13, 2019

eddyb commented Feb 14, 2019 • edited Loading

petrochenkov commented Jan 16, 2019 •

edited

Loading

petrochenkov commented Feb 1, 2019 •

edited

Loading

eddyb commented Feb 2, 2019 •

edited

Loading

eddyb commented Feb 14, 2019 •

edited

Loading