Add namespace support for rename operation #12563

edwardw · 2014-02-26T06:35:28Z

Maintains two renaming tables, one for regular idents and one for
lifetime idents (a loop label is of lifetime kind) so that let bindings
and loop labels with same name won't clash. Also, the mtwt hygiene code
is now @-free.

Closes #12512.

huonw · 2014-02-26T06:44:31Z

@jbclements: feedback?

edwardw · 2014-02-26T15:23:11Z

Be warned that this change comes with cost. Just tried #10607 out:

$ echo 'fn main() { ' 'let _x: int; /*'{1..5000}'*/' '}'  |  rustc - -Z time-passes
...
time: 7.136 s   expansion
...
time: 0.031 s   resolution

$ echo 'fn main() { ' 'let _x: int; /*'{1..5000}'*/' '}'  |  /patched/rustc - -Z time-passes
...
time: 17.769 s  expansion
...
time: 10.569 s  resolution

alexcrichton · 2014-02-26T23:54:27Z

I'm excited to see work done in this area, thank you!

That performance regression seems quite worrisome, however, do you know why it's happening?

huonw · 2014-02-27T00:02:27Z

(I'm a strong r- on this with the performance regressions, in particular because I think #12512 (comment) would work without a huge hit, even if it's not quite as clean. FWIW, ast::visit actually does a similar thing with visit_fn.)

However, without examining the code yet, I imagine you just may have accidentally added a source of O(n^2) (or worse) behaviour.

huonw · 2014-02-27T00:09:08Z

src/libsyntax/ast_util.rs

+                Some(ts) => ts.clone()
+            };
+            tables.borrow().with_mut(|ts|
+                ts.find_or_insert_with(ns, |_| Rc::new(RefCell::new(new_sctable_internal())))


I don't quite understand why we need a hashmap of two things.

struct SCTables { lifetime: Rc<RefCell<SCTable>> plain: Rc<RefCell<SCTable>> }

should work. (Stored as just SCTables in TLD.)

(I imagine this is most of the performance problem: hashing 1 bits has a lot of overhead.)

I'll experiment with this. I was anticipating the possible (now seems to be highly unlikely) event of adding a third IdentKind so that SCTables must keep synchronised. With hashmap it'll do so automatically. But of course you are right; the performance penalty is just too much.

If you select the field with a match like

match ns { PlainIdent => ..., LifetimeIdent => ..., }

then the addition of a new IdentKind will cause compilation to fail unless these are updated too (not quite automatic, but not a risk of a bug).

Stupid me :)

edwardw · 2014-02-27T18:52:45Z

Turns out in the middle of recursion, I replaced f(get_a(x)) with x.with_a(f). My wild guess is somehow f is not at the tail position any more and LLVM is prevented from TCO. If such speculation has any merit, then we are talking about potential O(2ⁿ). How often does that happen? I'm amazed that rust can compile itself at all without blowing up the stack. :D

Also @huonw, I did take your advice #12512 (comment) and added an additional argument to fold_ident. It's just harder to do the same at fold_expr level since all fold_tts knows is identifier.

$ echo 'fn main() { ' 'let _x: int; /*'{1..5000}'*/' '}'  |  /patched/rustc - -Z time-passes
...
time: 6.803 s   expansion
...
time: 0.029 s   resolution

r? @huonw @alexcrichton

huonw · 2014-02-27T23:35:15Z

Turns out in the middle of recursion, I replaced f(get_a(x)) with x.with_a(f). My wild guess is somehow f is not at the tail position any more and LLVM is prevented from TCO

Does this really change it significantly? (If you feel like it, it might be interesting to revert back to the with_a call to see if it's actually making a difference.)

It's just harder to do the same at fold_expr level since all fold_tts knows is identifier.

I don't understand what you mean by this sentence.

(Also, it looks like fold needs more LifetimeIdents: e.g. fn foo<'a>(...) should be folding that a as a lifetime.)

edwardw · 2014-02-28T05:56:35Z

Does this really change it significantly? (If you feel like it, it might be interesting to revert back to the with_a call to see if it's actually making a difference.)

Yes, it did. It was the offender and surprisingly, HashMap doesn't hurt very much.

It's just harder to do the same at fold_expr level since all fold_tts knows is identifier.

I meant fold_tts can't make use of fold_expr. If wen don't deal with the rename namespace at the fold_ident level, the very leaf, then other folder implementations may be forced to always think about expr, tts and ident too.

At the moment, I don't handle the 'real' lifetime folding at all, just the loop labels.

huonw · 2014-02-28T07:14:10Z

src/libsyntax/ast_util.rs

@@ -651,248 +649,296 @@ pub fn pat_is_ident(pat: @ast::Pat) -> bool {
 }

 // HYGIENE FUNCTIONS
+pub mod mtwt {


I feel like this could move to be either syntax::mtwt or syntax::ext::mtwt rather than squirrelled away in this rather mix-and-match "util" module.

Maybe syntax::mtwt, syntax::ext are mostly macro definitions.

Well... the whole expansion and hygiene renaming phases happen in syntax::ext.

Sure, syntax::ext it is.

Also, while you're at it, adding a doc comment along the lines of

//! Machinery for hygienic macros, as described in the MTWT[1] paper. //! //! [1] Matthew Flatt, Ryan Culpepper, David Darais, and Robert Bruce Findler. 2012. *Macros that work together: Compile-time bindings, partial expansion, and definition contexts*. J. Funct. Program. 22, 2 (March 2012), 181-216. DOI=10.1017/S0956796812000093 http://dx.doi.org/10.1017/S0956796812000093

(with more line breaks... ;) )

huonw · 2014-02-28T08:18:35Z

This seems very reasonable to me (the idea as well as the implementation), but I'd like to just get some confirmation from @jbclements that this isn't going to fundamentally break our macros (or anyone else reasonably familiar with it).

edwardw · 2014-02-28T17:50:50Z

The mtwt hygiene code moved to syntax::ext::mtwt and all comments, I believe, addressed. There's even a bonus:

$ echo 'fn main() { ' 'let _x: int; /*'{1..5000}'*/' '}' | rustc - -Z time-passes
...
time: 6.745 s   expansion
$ echo 'fn main() { ' 'let _x: int; /*'{1..5000}'*/' '}' | /patched/rustc - -Z time-passes
...
time: 3.723 s   expansion

It could come from other (unrelated) patches I pulled in, though :D Seriously, it should have something to do with fixing FIXME 5074, which improves mtwt::new_rename and mtwt::new_mark.

huonw · 2014-03-01T06:27:13Z

src/libsyntax/ext/mtwt.rs

+                    table: &SCTable,
+                    resolve_table: &mut ResolveTable) -> Name {
+    let key = (id.name, id.ctxt);
+    match resolve_table.contains_key(&key) {


I wonder if there would be some speed gains in resolve by writing this as

match resolve_table.find(&key) { Some(&name) => return name, None => {} } let resolved = { ... }; ...

(to only hash key once; I have a strong feeling that hashing once rather than twice was a major contributor to the speed-up seen on the #10607 smoke-test.)

I'm working on it. It is exclusively mutably borrowed and recursive so the find_or_insert_with trick doesn't seem to work right away.

I think the early return is as good as you'll do here. :) (And it's certainly the lowest hanging fruit.)

The resolve time was dozen milliseconds so -Z time-passes is not able to properly gauge speed gains here, not statistically convincingly at least. Anyway, I gonna check this in; the code is much better that way.

huonw · 2014-03-01T06:30:19Z

Awesome work on the last commit! (Code that's shorter, simpler and faster!)

edwardw · 2014-03-01T18:51:22Z

Another 2x speed gain by specializing hasher for mtwt tables.

huonw · 2014-03-01T22:29:06Z

(This needs a rebase too.)

flaper87 · 2014-03-03T13:07:22Z

great work here! 👍

edwardw · 2014-03-03T18:25:06Z

Rebased. There was a massive change from ~[T] to Vec<T>, which made this round of rebasing rather painful. The performance also suffered a bit because of it.

huonw · 2014-03-03T20:40:27Z

The performance also suffered a bit because of it.

I'm... surprised by this: theoretically Vec is faster... without looking at the code again, the only thing I can think of is #11751.

huonw · 2014-03-03T20:43:01Z

src/libsyntax/ext/mtwt.rs

+fn new_mark_internal(m: Mrk, tail: SyntaxContext, table: &SCTable) -> SyntaxContext {
+    let key = MarkKey { mrk: m, tail: tail };
+    let mut mark_memo = table.mark_memo.borrow_mut();
+    let new_ctxt = || idx_push(table.table.borrow_mut().get(), Mark(key));


FWIW, why is there two layers of closures here? Couldn't this just be inlined in to the |_| ...?

Good catch.

I have to say:

let new_ctxt = |_: &(SyntaxContext, Mrk)| ... *mark_memo.get().find_or_insert_with(key, new_ctxt)

The type inference get choked here :)

huonw · 2014-03-03T20:47:56Z

Ok, looked over it; there's nothing obvious about why it would be particularly slower.

Also, I thought about it a bit, and I'm not so keen on the custom hasher: it does give good speed-ups, but it might be better to wait until the centralised hasher has been optimised some more before we start overriding default type params and generally spreading Hasher impls through out the codebase (#12635 (comment)).

edwardw · 2014-03-04T06:58:01Z

Ok, I removed the last commit about specialising hasher.

Regarding the slight performance issue, the vector element in question is a tuple (Ident, Name), 12 bytes in memory. The new vec_ng always doubles the vector size if running out of capacity, while vec only increases it modestly by 1. In the hygiene let smoke test, there is 5000 elements in the renaming list at the peak. So vec_ng has 8192 capacity or 8192 * 12 = 96k, vec on the other hand only uses roundup(5000 * 12) = 64k, it could be that vec is more CPU L1/L2 cache friendly in this case.

In any case, the performance difference is not that significant.

huonw · 2014-03-04T07:04:46Z

while vec only increases it modestly by 1

I don't believe that is true: .push calls .reserve_additional which calls .reserve which rounds up to the next power of two. i.e. it does double.

(Increasing the capacity by just 1 each time would cause quadratic runtime, due to having to realloc O(n) times.)

edwardw · 2014-03-04T07:08:08Z

You are right, I should have been more thorough.

edwardw · 2014-03-04T16:50:01Z

To be absolutely certain, I reverted Vec<T> to ~[T] on top the rebased commits and run smoke test again. The performance difference is neglectable; I must've seen some ghost numbers.

edwardw · 2014-03-05T07:30:37Z

@jbclements comments?

jbclements · 2014-03-05T07:34:42Z

Aw, geez. This whole thread is something I really have not had the time to keep up with. You finally got my attention, though, so I have at least one big question. Why do you need separate tables for lifetimes? Can you show me a concrete example where having a single table would cause a problem? I believe that the existing setup handles different kinds of bindings pretty transparently.

edwardw · 2014-03-05T07:46:05Z

Here is the example from the original bug report:

fn main() {
    let mut foo = ~[];
    'foo: for i in [1, 2, 3].iter() {
        foo.push(i);
    }
}

One consequence of adding hygiene label support is that now label foo will shadow local var foo. So I come up with the idea of managing a separate table for lifetime since loop labels are lifetimes at token level. That way, local vars and loop labels with same name will not clash.

jbclements · 2014-03-05T07:48:41Z

no... why would label foo shadow the let binding? One's a binding, one's a lifetime. The lifetime shouldn't end up in an environment during name resolution. I'm worried that something is seriously wrong here.

edwardw · 2014-03-05T07:51:31Z

Loop labels are lifetime at token level, but they become ast::Ident at AST level. So the label renamings will show up in the pending renaming list and be picked up by any other Ident in the loop body.

jbclements · 2014-03-05T08:01:24Z

@edwardw Grr... okay, gears grinding. You may be right. Lemme think about it more.

jbclements · 2014-03-05T08:15:41Z

@edwardw Okay, added a lengthy-ish comment to issue #12512 . Let me know if it makes sense to you.

huonw · 2014-03-05T08:39:13Z

@edwardw If you felt like it, you could probably split out the general fixes commits of this and submit them as a separate PR, so that the perf improvements & FIXME-removals aren't blocked on the decision about #12512.

edwardw · 2014-03-05T14:50:09Z

I split general improvements over the mtwt code into #12711.

Maintains two renaming tables, one for regular idents and one for lifetime idents (a loop label is of lifetime kind) so that let bindings and loop labels with same name won't clash. Closes rust-lang#12512.

alexcrichton · 2014-03-16T05:50:57Z

Closing due to inactivity. It sounds like the way to solve #12512 has a few options, and the solution may need to get settled on before making more progress.

It seems like a serious bug though, and I'd love to see it fixed!

internal: Simplify

make sure checked type implements `Try` trait when linting [`question_mark`] (indirectly) fixes: rust-lang#12412 and fixes: rust-lang#11983 --- changelog: make sure checked type implements `Try` trait when linting [`question_mark`]

huonw reviewed Feb 27, 2014
View reviewed changes

huonw reviewed Feb 28, 2014
View reviewed changes

edwardw mentioned this pull request Feb 28, 2014

Macro expansion is quadratic in the number of let statements. #10607

Closed

huonw reviewed Mar 1, 2014
View reviewed changes

huonw reviewed Mar 3, 2014
View reviewed changes

Add namespace support for rename operation

211012b

Maintains two renaming tables, one for regular idents and one for lifetime idents (a loop label is of lifetime kind) so that let bindings and loop labels with same name won't clash. Closes rust-lang#12512.

alexcrichton mentioned this pull request Mar 9, 2014

Loop label hides variables with same name #12512

Closed

alexcrichton closed this Mar 16, 2014

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022

Auto merge of rust-lang#12563 - Veykril:completion, r=Veykril

9c0b727

internal: Simplify

Add namespace support for rename operation #12563

Add namespace support for rename operation #12563

Conversation

edwardw commented Feb 26, 2014

huonw commented Feb 26, 2014

edwardw commented Feb 26, 2014

alexcrichton commented Feb 26, 2014

huonw commented Feb 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardw commented Feb 27, 2014

huonw commented Feb 27, 2014

edwardw commented Feb 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Feb 28, 2014

edwardw commented Feb 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Mar 1, 2014

edwardw commented Mar 1, 2014

huonw commented Mar 1, 2014

flaper87 commented Mar 3, 2014

edwardw commented Mar 3, 2014

huonw commented Mar 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Mar 3, 2014

edwardw commented Mar 4, 2014

huonw commented Mar 4, 2014

edwardw commented Mar 4, 2014

edwardw commented Mar 4, 2014

edwardw commented Mar 5, 2014

jbclements commented Mar 5, 2014

edwardw commented Mar 5, 2014

jbclements commented Mar 5, 2014

edwardw commented Mar 5, 2014

jbclements commented Mar 5, 2014

jbclements commented Mar 5, 2014

huonw commented Mar 5, 2014

edwardw commented Mar 5, 2014

alexcrichton commented Mar 16, 2014