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

Crazy JVM stack requirements for compiling big-ish case class? #12397

Closed
lihaoyi opened this issue May 16, 2021 · 0 comments · Fixed by scala/scala#9635
Closed

Crazy JVM stack requirements for compiling big-ish case class? #12397

lihaoyi opened this issue May 16, 2021 · 0 comments · Fixed by scala/scala#9635
Labels
Milestone

Comments

@lihaoyi
Copy link

lihaoyi commented May 16, 2021

reproduction steps

Full code and repro commands:

$ cat mill
#!/usr/bin/env sh

# This is a wrapper script, that automatically download mill from GitHub release pages
# You can give the required mill version with MILL_VERSION env variable
# If no version is given, it falls back to the value of DEFAULT_MILL_VERSION
DEFAULT_MILL_VERSION=0.9.5-52-ef6d5d

set -e

if [ -z "$MILL_VERSION" ] ; then
  if [ -f ".mill-version" ] ; then
    MILL_VERSION="$(head -n 1 .mill-version 2> /dev/null)"
  elif [ -f "mill" ] && [ "$BASH_SOURCE" != "mill" ] ; then
    MILL_VERSION=$(grep -F "DEFAULT_MILL_VERSION=" "mill" | head -n 1 | cut -d= -f2)
  else
    MILL_VERSION=$DEFAULT_MILL_VERSION
  fi
fi

if [ "x${XDG_CACHE_HOME}" != "x" ] ; then
  MILL_DOWNLOAD_PATH="${XDG_CACHE_HOME}/mill/download"
else
  MILL_DOWNLOAD_PATH="${HOME}/.cache/mill/download"
fi
MILL_EXEC_PATH="${MILL_DOWNLOAD_PATH}/${MILL_VERSION}"

version_remainder="$MILL_VERSION"
MILL_MAJOR_VERSION="${version_remainder%%.*}"; version_remainder="${version_remainder#*.}"
MILL_MINOR_VERSION="${version_remainder%%.*}"; version_remainder="${version_remainder#*.}"

if [ ! -x "$MILL_EXEC_PATH" ] ; then
  mkdir -p $MILL_DOWNLOAD_PATH
  if [ "$MILL_MAJOR_VERSION" -gt 0 ] || [ "$MILL_MINOR_VERSION" -ge 5 ] ; then
    ASSEMBLY="-assembly"
  fi
  DOWNLOAD_FILE=$MILL_EXEC_PATH-tmp-download
  MILL_DOWNLOAD_URL="https://github.com/lihaoyi/mill/releases/download/${MILL_VERSION%%-*}/$MILL_VERSION${ASSEMBLY}"
  curl --fail -L -o "$DOWNLOAD_FILE" "$MILL_DOWNLOAD_URL"
  chmod +x "$DOWNLOAD_FILE"
  mv "$DOWNLOAD_FILE" "$MILL_EXEC_PATH"
  unset DOWNLOAD_FILE
  unset MILL_DOWNLOAD_URL
fi

unset MILL_DOWNLOAD_PATH
unset MILL_VERSION

exec $MILL_EXEC_PATH "$@"

lihaoyi test$ cat build.sc
import mill.scalalib._

object test extends ScalaModule{
  def scalaVersion = "2.13.4"
}

lihaoyi test$ cat test/src/Big150.scala

case class Big150(_0: Int, _1: Int, _2: Int, _3: Int, _4: Int, _5: Int, _6: Int, _7: Int,
                  _8: Int, _9: Int, _10: Int, _11: Int, _12: Int, _13: Int, _14: Int,
                  _15: Int, _16: Int, _17: Int, _18: Int, _19: Int, _20: Int, _21: Int,
                  _22: Int, _23: Int, _24: Int, _25: Int, _26: Int, _27: Int, _28: Int,
                  _29: Int, _30: Int, _31: Int, _32: Int, _33: Int, _34: Int, _35: Int,
                  _36: Int, _37: Int, _38: Int, _39: Int, _40: Int, _41: Int, _42: Int,
                  _43: Int, _44: Int, _45: Int, _46: Int, _47: Int, _48: Int, _49: Int,
                  _50: Int, _51: Int, _52: Int, _53: Int, _54: Int, _55: Int, _56: Int,
                  _57: Int, _58: Int, _59: Int, _60: Int, _61: Int, _62: Int, _63: Int,
                  _64: Int, _65: Int, _66: Int, _67: Int, _68: Int, _69: Int, _70: Int,
                  _71: Int, _72: Int, _73: Int, _74: Int, _75: Int, _76: Int, _77: Int,
                  _78: Int, _79: Int, _80: Int, _81: Int, _82: Int, _83: Int, _84: Int,
                  _85: Int, _86: Int, _87: Int, _88: Int, _89: Int, _90: Int, _91: Int,
                  _92: Int, _93: Int, _94: Int, _95: Int, _96: Int, _97: Int, _98: Int,
                  _99: Int, _100: Int, _101: Int, _102: Int, _103: Int, _104: Int,
                  _105: Int, _106: Int, _107: Int, _108: Int, _109: Int, _110: Int,
                  _111: Int, _112: Int, _113: Int, _114: Int, _115: Int, _116: Int,
                  _117: Int, _118: Int, _119: Int, _120: Int, _121: Int, _122: Int,
                  _123: Int, _124: Int, _125: Int, _126: Int, _127: Int, _128: Int,
                  _129: Int, _130: Int, _131: Int, _132: Int, _133: Int, _134: Int,
                  _135: Int, _136: Int, _137: Int, _138: Int, _139: Int, _140: Int,
                  _141: Int, _142: Int, _143: Int, _144: Int, _145: Int, _146: Int,
                  _147: Int, _148: Int, _149: Int)

lihaoyi test$ JAVA_OPTS=-Xss1024M ./mill test.compile
[26/26] test.compile
[info] compiling 1 Scala source to /Users/lihaoyi/Github/test/out/test/compile/dest/classes ...
[error] ## Exception when compiling 1 sources to /Users/lihaoyi/Github/test/out/test/compile/dest/classes
[error] java.lang.StackOverflowError

The exact failure seems non-deterministic. I've had it compile before with the default stack size (1mb?), and have had it pass before with 996mb, but right now i can't get it to compile even with 1024mb of stack, which is the limit the JVM allows you to set.

Making it a normal non-case class seems to make the problem go away, as does adding an override def equals(other: Any): Boolean = false

It feels like the code being auto-generated for the case class is probably making the compiler work too hard at inferring something? Or maybe we're generating deeply imbalanced ASTs in the .equals or .hashCode implementation that can be fixed by adding some strategic parens to group things into a balanced tree of O(log n) depth instead of O(n)?

I can accept that bigger programs/expressions/definitions may require more resources, but requiring >1024mb of stack space is unreasonable.

I bumped into this in the context of com-lihaoyi/upickle#348, where I'm trying to make my JSON library support large case classes, e.g. modelling Github's "repository" JSON response which has ~450 fields. Given this minimal repro falls apart at 150 fields, there's clearly a long way to go to support 450!

@SethTisue SethTisue added this to the 2.13.7 milestone May 16, 2021
lihaoyi added a commit to lihaoyi/scala that referenced this issue May 18, 2021
…hetic `def equals` method:

```scala
a && b && c && d && e && f && g && h
```

Which currently parses into an unbalanced depth O(n) tree as follows:

```scala
(((((((a && b) && c) && d) && e) && f) && g) && h)
```

into a binary tree of depth O(log n):

```scala
(((a && b) && (c && d)) && ((e && f) && (g && h)))
```

Tested manually by pasting the following snippet into the `sbt scala` interpreter:

```scala
case class Big150(_0: Int, _1: Int, _2: Int, _3: Int, _4: Int, _5: Int, _6: Int, _7: Int,
                  _8: Int, _9: Int, _10: Int, _11: Int, _12: Int, _13: Int, _14: Int,
                  _15: Int, _16: Int, _17: Int, _18: Int, _19: Int, _20: Int, _21: Int,
                  _22: Int, _23: Int, _24: Int, _25: Int, _26: Int, _27: Int, _28: Int,
                  _29: Int, _30: Int, _31: Int, _32: Int, _33: Int, _34: Int, _35: Int,
                  _36: Int, _37: Int, _38: Int, _39: Int, _40: Int, _41: Int, _42: Int,
                  _43: Int, _44: Int, _45: Int, _46: Int, _47: Int, _48: Int, _49: Int,
                  _50: Int, _51: Int, _52: Int, _53: Int, _54: Int, _55: Int, _56: Int,
                  _57: Int, _58: Int, _59: Int, _60: Int, _61: Int, _62: Int, _63: Int,
                  _64: Int, _65: Int, _66: Int, _67: Int, _68: Int, _69: Int, _70: Int,
                  _71: Int, _72: Int, _73: Int, _74: Int, _75: Int, _76: Int, _77: Int,
                  _78: Int, _79: Int, _80: Int, _81: Int, _82: Int, _83: Int, _84: Int,
                  _85: Int, _86: Int, _87: Int, _88: Int, _89: Int, _90: Int, _91: Int,
                  _92: Int, _93: Int, _94: Int, _95: Int, _96: Int, _97: Int, _98: Int,
                  _99: Int, _100: Int, _101: Int, _102: Int, _103: Int, _104: Int,
                  _105: Int, _106: Int, _107: Int, _108: Int, _109: Int, _110: Int,
                  _111: Int, _112: Int, _113: Int, _114: Int, _115: Int, _116: Int,
                  _117: Int, _118: Int, _119: Int, _120: Int, _121: Int, _122: Int,
                  _123: Int, _124: Int, _125: Int, _126: Int, _127: Int, _128: Int,
                  _129: Int, _130: Int, _131: Int, _132: Int, _133: Int, _134: Int,
                  _135: Int, _136: Int, _137: Int, _138: Int, _139: Int, _140: Int,
                  _141: Int, _142: Int, _143: Int, _144: Int, _145: Int, _146: Int,
                  _147: Int, _148: Int, _149: Int)
```

This semi-reliably crashes the interpreter with a StackOverflow on 2.13.x, and works without issue on this PR.

I'm not sure where the tests should go, but let me know and I'll happily paste that snippet into your test suite (or you guys could do it on my behalf when merging!)

It's not clear to me if the other generated methods suffer the same unbalanced-AST issue, but glancing over the code it seems they don't: e.g. `.hashCode` has a long chain of `val` assignments of AST depth O(1), `.productElement` is one big pattern match of depth O(1), etc. The fact that this seems to fix the StackOverflow without it turning up somewhere else also supports the idea that `.equals` is the only generated method with this issue

Seems the problematic behavior was introduced 14 years ago in scala@8397c7b#diff-205537ac4c08ea690ada72e398df0018dcaf2a7c4987c0d8d8df322314469578R162
lihaoyi added a commit to lihaoyi/scala that referenced this issue May 18, 2021
…hetic `def equals` method:

```scala
a && b && c && d && e && f && g && h
```

Which currently parses into an unbalanced depth O(n) tree as follows:

```scala
(((((((a && b) && c) && d) && e) && f) && g) && h)
```

into a binary tree of depth O(log n):

```scala
(((a && b) && (c && d)) && ((e && f) && (g && h)))
```

Tested manually by pasting the following snippet into the `sbt scala` interpreter:

```scala
case class Big150(_0: Int, _1: Int, _2: Int, _3: Int, _4: Int, _5: Int, _6: Int, _7: Int,
                  _8: Int, _9: Int, _10: Int, _11: Int, _12: Int, _13: Int, _14: Int,
                  _15: Int, _16: Int, _17: Int, _18: Int, _19: Int, _20: Int, _21: Int,
                  _22: Int, _23: Int, _24: Int, _25: Int, _26: Int, _27: Int, _28: Int,
                  _29: Int, _30: Int, _31: Int, _32: Int, _33: Int, _34: Int, _35: Int,
                  _36: Int, _37: Int, _38: Int, _39: Int, _40: Int, _41: Int, _42: Int,
                  _43: Int, _44: Int, _45: Int, _46: Int, _47: Int, _48: Int, _49: Int,
                  _50: Int, _51: Int, _52: Int, _53: Int, _54: Int, _55: Int, _56: Int,
                  _57: Int, _58: Int, _59: Int, _60: Int, _61: Int, _62: Int, _63: Int,
                  _64: Int, _65: Int, _66: Int, _67: Int, _68: Int, _69: Int, _70: Int,
                  _71: Int, _72: Int, _73: Int, _74: Int, _75: Int, _76: Int, _77: Int,
                  _78: Int, _79: Int, _80: Int, _81: Int, _82: Int, _83: Int, _84: Int,
                  _85: Int, _86: Int, _87: Int, _88: Int, _89: Int, _90: Int, _91: Int,
                  _92: Int, _93: Int, _94: Int, _95: Int, _96: Int, _97: Int, _98: Int,
                  _99: Int, _100: Int, _101: Int, _102: Int, _103: Int, _104: Int,
                  _105: Int, _106: Int, _107: Int, _108: Int, _109: Int, _110: Int,
                  _111: Int, _112: Int, _113: Int, _114: Int, _115: Int, _116: Int,
                  _117: Int, _118: Int, _119: Int, _120: Int, _121: Int, _122: Int,
                  _123: Int, _124: Int, _125: Int, _126: Int, _127: Int, _128: Int,
                  _129: Int, _130: Int, _131: Int, _132: Int, _133: Int, _134: Int,
                  _135: Int, _136: Int, _137: Int, _138: Int, _139: Int, _140: Int,
                  _141: Int, _142: Int, _143: Int, _144: Int, _145: Int, _146: Int,
                  _147: Int, _148: Int, _149: Int)
```

This semi-reliably crashes the interpreter with a StackOverflow on 2.13.x, and works without issue on this PR.

I'm not sure where the tests should go, but let me know and I'll happily paste that snippet into your test suite (or you guys could do it on my behalf when merging!)

It's not clear to me if the other generated methods suffer the same unbalanced-AST issue, but glancing over the code it seems they don't: e.g. `.hashCode` has a long chain of `val` assignments of AST depth O(1), `.productElement` is one big pattern match of depth O(1), etc. The fact that this seems to fix the StackOverflow without it turning up somewhere else also supports the idea that `.equals` is the only generated method with this issue

Seems the problematic behavior was introduced 14 years ago in scala@8397c7b#diff-205537ac4c08ea690ada72e398df0018dcaf2a7c4987c0d8d8df322314469578R162
lihaoyi added a commit to lihaoyi/scala that referenced this issue May 18, 2021
…hetic `def equals` method:

```scala
a && b && c && d && e && f && g && h
```

Which currently parses into an unbalanced depth O(n) tree as follows:

```scala
(((((((a && b) && c) && d) && e) && f) && g) && h)
```

into a binary tree of depth O(log n):

```scala
(((a && b) && (c && d)) && ((e && f) && (g && h)))
```

Tested manually by pasting the following snippet into the `sbt scala` interpreter:

```scala
case class Big150(_0: Int, _1: Int, _2: Int, _3: Int, _4: Int, _5: Int, _6: Int, _7: Int,
                  _8: Int, _9: Int, _10: Int, _11: Int, _12: Int, _13: Int, _14: Int,
                  _15: Int, _16: Int, _17: Int, _18: Int, _19: Int, _20: Int, _21: Int,
                  _22: Int, _23: Int, _24: Int, _25: Int, _26: Int, _27: Int, _28: Int,
                  _29: Int, _30: Int, _31: Int, _32: Int, _33: Int, _34: Int, _35: Int,
                  _36: Int, _37: Int, _38: Int, _39: Int, _40: Int, _41: Int, _42: Int,
                  _43: Int, _44: Int, _45: Int, _46: Int, _47: Int, _48: Int, _49: Int,
                  _50: Int, _51: Int, _52: Int, _53: Int, _54: Int, _55: Int, _56: Int,
                  _57: Int, _58: Int, _59: Int, _60: Int, _61: Int, _62: Int, _63: Int,
                  _64: Int, _65: Int, _66: Int, _67: Int, _68: Int, _69: Int, _70: Int,
                  _71: Int, _72: Int, _73: Int, _74: Int, _75: Int, _76: Int, _77: Int,
                  _78: Int, _79: Int, _80: Int, _81: Int, _82: Int, _83: Int, _84: Int,
                  _85: Int, _86: Int, _87: Int, _88: Int, _89: Int, _90: Int, _91: Int,
                  _92: Int, _93: Int, _94: Int, _95: Int, _96: Int, _97: Int, _98: Int,
                  _99: Int, _100: Int, _101: Int, _102: Int, _103: Int, _104: Int,
                  _105: Int, _106: Int, _107: Int, _108: Int, _109: Int, _110: Int,
                  _111: Int, _112: Int, _113: Int, _114: Int, _115: Int, _116: Int,
                  _117: Int, _118: Int, _119: Int, _120: Int, _121: Int, _122: Int,
                  _123: Int, _124: Int, _125: Int, _126: Int, _127: Int, _128: Int,
                  _129: Int, _130: Int, _131: Int, _132: Int, _133: Int, _134: Int,
                  _135: Int, _136: Int, _137: Int, _138: Int, _139: Int, _140: Int,
                  _141: Int, _142: Int, _143: Int, _144: Int, _145: Int, _146: Int,
                  _147: Int, _148: Int, _149: Int)
```

This semi-reliably crashes the interpreter with a StackOverflow on 2.13.x, and works without issue on this PR.

I'm not sure where the tests should go, but let me know and I'll happily paste that snippet into your test suite (or you guys could do it on my behalf when merging!)

It's not clear to me if the other generated methods suffer the same unbalanced-AST issue, but glancing over the code it seems they don't: e.g. `.hashCode` has a long chain of `val` assignments of AST depth O(1), `.productElement` is one big pattern match of depth O(1), etc. The fact that this seems to fix the StackOverflow without it turning up somewhere else also supports the idea that `.equals` is the only generated method with this issue

Seems the problematic behavior was introduced 14 years ago in scala@8397c7b#diff-205537ac4c08ea690ada72e398df0018dcaf2a7c4987c0d8d8df322314469578R162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants