Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coffee -c fails on moderately large .coffee files in v2.6.0 (worked in 2.5 and earlier) #5378

Closed
rodw opened this issue Sep 26, 2021 · 5 comments
Labels

Comments

@rodw
Copy link

rodw commented Sep 26, 2021

Compilation (via coffee -c) in v2.6.0 seems to fail on large(-ish) .coffee files that worked in v2.5.1 and previous releases. (And that are well within a range one could reasonably expect to work IMO.)

I have an electron app implemented in CoffeeScript. As part of my build process I concatenate a large number of .coffee files into a single renderer.coffee, and then compile with coffee -c renderer.coffee to generate renderer.js, which is the file loaded by my root HTML page in electron.

This process has worked well for a couple of years at least. However when I attempted to upgrade to CoffeeScript v2.6 I found that coffee -c renderer.coffee is now failing with an error like the following

RangeError: Maximum call stack size exceeded
    at Root.compileNode ([...]/node_modules/coffeescript/lib/coffeescript/nodes.js:740:15)
    at Root.compileToFragments ([...]/node_modules/coffeescript/lib/coffeescript/nodes.js:143:74)
    at Object.<anonymous> ([...]/node_modules/coffeescript/lib/coffeescript/coffeescript.js:166:23)
    at Object.<anonymous> ([...]/node_modules/coffeescript/lib/coffeescript/coffeescript.js:54:19)
    at Object.CoffeeScript.compile ([...]/node_modules/coffeescript/lib/coffeescript/index.js:45:29)
    at compileScript ([...]/node_modules/coffeescript/lib/coffeescript/command.js:293:33)
    at compilePath ([...]/node_modules/coffeescript/lib/coffeescript/command.js:237:14)
    at Object.exports.run ([...]/node_modules/coffeescript/lib/coffeescript/command.js:158:20)
    at Object.<anonymous> ([...]/node_modules/coffeescript/bin/coffee:22:45)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    [...]

I believe I've discovered a relatively simple example that reproduces this problem at just under 7000 lines.

Take the following script:

# FILE: large-coffee-generator.coffee
max = 6969
console.log "i = 0"
for i in [0...max]
  console.log "console.log \"Line \#{i++}\""

Running that program (e.g., coffee large-coffee-generator.coffee > foo.coffee) will generate a file that looks something like:

# FILE: foo.coffee
i = 0
console.log "Line #{i++}"
# ...etc....
console.log "Line #{i++}"

where that last line is repeated max times for a total of max + 1 lines.

Now try to compile that output via coffee -c foo.coffee

  • With CoffeeScript v2.5.1 this works (somewhat slowly) with max values of 40,000 or more.

  • With CoffeeScript v2.6.0 this works for max = 6968 - running in about 8s in on my MacBook Pro laptop - but fails for max = 6969. I.e. it starts to fail with a "Maximum call stack size exceeded" error at precisely 6970 lines.

For context:

  • I'm running this on a modernish MacBook Pro: 2 GHz Quad-Core Intel Core i5 with 32 GB RAM

  • I'm using Node v14.16.0 (the latest stable release)

  • I'm using CoffeeScript v2.6.0 (the latest stable release)

@GeoffreyBooth
Copy link
Collaborator

GeoffreyBooth commented Sep 27, 2021

cc @edemaine @helixbass I’d guess that this was caused by one of the three PRs in #5374.

@rodw if you have time, you can narrow things down to the commit where the issue was introduced by checking out the CoffeeScript repo, building locally, and stepping back commit by commit until the problem goes away (which should be the commit with 2.5.1, if not somewhere between that and 2.6.0). There are only ten commits between the versions, though several of them can be ignored as they’re changes to documentation or CI.

GeoffreyBooth added a commit to GeoffreyBooth/coffeescript that referenced this issue Oct 2, 2021
@GeoffreyBooth
Copy link
Collaborator

@rodw I tried your test on my machine. Under 2.5.1 the file with 6969 lines compiled for me in 9 seconds. Under 2.6.0 it also throws with Maximum call stack size exceeded.

I went back commit by commit on master. Compilation failed until I got to commit b0946c3, which means that the bug was introduced in c4f1fe7: #5371, adding top-level await.

I tried compiling the test file with --bare, and curiously, that made the error disappear. The code that is influenced by --bare and that changed in #5371 are these lines: https://github.com/jashkenas/coffeescript/pull/5371/files#diff-755365db3eccb47ea8933fe738a487f884fc46ba183dfef30e690d79915f662aR530-R536. I rewrote them to be much more similar to the previous version and the bug went away.

I wrote a test for this, along the lines of the reproduction steps, but it tripled the time it took for the tests to run so I removed it. It we can think of a test that can validate that this doesn’t break in the future, and takes < 200 ms or so on its own to run, that would be good to add.

@edemaine
Copy link
Contributor

edemaine commented Oct 2, 2021

@GeoffreyBooth Thanks for tracking this down, and fixing it! The new code looks good. Alternatively, I think parts.push ...fragments could be replaced by parts.push fragments; I'm not sure why I wrote it that way. (I was confused by their equivalence, but now I see that non-array arguments passed to [].concat get converted to singleton arrays.)

Apologies for introducing the performance bug. I forgot (having learned only recently) that func(...args) is terrible when args is large, as it puts every argument onto the stack. (I've spent too long thinking that array.push(...parts) is equivalent to Python's array.extend(parts).)

I also couldn't find an example for testing that has a lot of top-level lines and isn't really slow to compile. 🙁

GeoffreyBooth added a commit to GeoffreyBooth/coffeescript that referenced this issue Oct 2, 2021
@GeoffreyBooth
Copy link
Collaborator

I think the reason this probably didn’t affect more people is that this only happens when bare mode is not used. --bare isn’t enabled by default when using coffee but most build tools have it enabled by default, since they use their own logic for bundling. The key part of @rodw’s bug report is “As part of my build process I concatenate a large number of .coffee files into a single renderer.coffee”—thank you @rodw for such a detailed explanation!

@rodw
Copy link
Author

rodw commented Oct 6, 2021

I finally got around to testing my app with this fix and everything is working fine under Coffee 2.6.1. Thanks to @GeoffreyBooth et al for the quick fix. Looking forward to taking advantage of the 2.6 features!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants