Stack Overflow

[FreeMasen]

Currently these tests panic at 6 levels of nesting, in debug mode.

function x() {
    return function() {
        return function() {
            return function() {
                return function() {
                    return function() {
                    }
                }
            }
        }
    }
}

Especially with how modern js packaging works this is not an uncommon level of nesting.

The only way to get our tests to pass currently is to use the environment variable RUST_MIN_STACK set to 9999999 (7 nines) or run in release mode (the moz_central tests fail in release mode). There are 2 potential fixes for this issues

Options

1. Reduce recursion
2. Integrate the stacker crate to dynamically increase the stack size in a configurable way

Option 1

Technically "any recursive operation can be flattened into a while loop" but that doesn't mean it will be easy to do, this would essentially be a full re-write of this crate. It might be time to tackle this as I also believe we would see better performance with change.

Option 2

While this has the potential to be a much smaller lift from the shear size of changes, it would mostly just push the problem onto consumers. Determining reasonable default is also a bit of a problem since poorly chosen defaults would cause performance regressions and/or ergonomic issues.

[j-mendez]

@FreeMasen there might be a condition preventing the recursion from exiting. Thank you for the crate!

The following example produces the overflow.

#[test]
fn stack_oof() {
    env_logger::builder().is_test(true).try_init().ok();
    run_test(
        r###"
        (function() {  
            var Choosealicense, 
            LicenseSuggestion, 
            bind = function(fn, me){ 
                return function(){ 
                    return fn.apply(me, arguments); 
                }; 
            };
            Choosealicense = (function() {
                Choosealicense.prototype.selectText = function(element) {
                  var range, selection;
        
                  if (document.body.createTextRange) {
        
                  } else if (window.getSelection) {
        
                  }
        
                };
        
            });
            
            })()
        "###,
        false,
    )
    .unwrap();
}

[FreeMasen]

Thank you very much for the new example! I will have to investigate further but I do find it interesting that your example also contains 7 open parentheses which is the same number as my example.

I think part of the problem is the sheer size of the ast nodes, I recently discovered how large the Cow type is and worry that it is more to blame than I previously had thought.

That isn't to say I haven't written an infinite loop on accident... I'll do some more digging this week/weekend

[j-mendez]

@FreeMasen no problem, another interesting thing is if we change that last else if to an else it works fine.

[FreeMasen]

I did a little digging this morning and didn't see anything stand out as an infinite loop, the tests will actually complete in --release mode. I started to try and instrument some stack size inspection but had to step away before I could get it working. Hopefully there will be more to report this weekend

[FreeMasen]

I haven't had a chance to instrument anything but I did throw together a crate that checks the size of the ast.

I think the primary issue is that the Stmt enum is 1.84k, which is huge, especially when compared with the un-spanned enum at 304b

[j-mendez]

@FreeMasen that would do it. I have a branch refactoring some of the stack allocations to heap. Simple stuff but, changes to the Stmt look like a good place to put a pr to maybe do the same?

[FreeMasen]

Yeah, changes to the Stmt is probably the best place to start. Potentially just having all of the struct variants Boxed. Another potential place to look for some savings is to adjust how we are dealing with SourceLocation since that ends up being 4 usize values but could be looking at how syn does it they use 2 u32s and a thread_local map for converting to a position.

[FreeMasen]

So, I went down the rabbit hole over the past few days about how to keep the existing ergonomics but reduce the size of the ast. My initial PR is up here, I was able to make some very serious savings, some of which might even be mostly backwards compatible.

Hopefully I'm not stepping on your toes here, I am always happy to have help with these projects

[j-mendez]

@FreeMasen appreciate the consideration, it is all good! I checked out the PR, the lifetime removal is a good change. Feel free to continue on, I will try to see if I can test the changes for improvements as well.