scala / scala-dev

Scala 2 team issues. Not for user-facing bugs or directly actionable user-facing improvements. For build/test/infra and for longer-term planning and idea tracking. Our bug tracker is at https://github.com/scala/bug/issues
Apache License 2.0
130 stars 15 forks source link

multiple PRs are green on Jenkins, red on Travis-CI #570

Closed SethTisue closed 5 years ago

SethTisue commented 5 years ago

(list has been edited based on comments below)

SethTisue commented 5 years ago

Lukas thought at least one of the PRs might be stack size related, not sure what his evidence is

probably no harm in just adding an -Xss to .jvmopts, now that we have it? the default stack size is liable to be a bit different on different systems, and is always rather small

Lukas Rytz [4 hours ago] i use 5m locally

milessabin commented 5 years ago

https://github.com/scala/scala/pull/7368 was a transient failure due to a failed fetch of a binary artefact from a Lightbend repo ... went green on Travis when rerun.

milessabin commented 5 years ago

One step forward, one step back ...

https://github.com/scala/scala/pull/7199 is now failing with a SOE on Travis, after rebasing. In this case at least, the failure is after bootstrapping.

retronym commented 5 years ago

Pattern matching is now generating additional null checks. I'm guessing this is an intended consequence of moving it after refchecks.

These extra null checks are doubling the tree depth of the translated patterns, which was already pretty big for some of the high-arity tuple matches in StandardLiftables.

I'll dig further.

image

retronym commented 5 years ago

Okay, that change comes down to a slightly older, intentional change to null handling with extractors: scala/scala#6485

Maybe we should change patmat to emit:

if (p5.eq(null) case12()
<next pattern>

Rather than:

if (p5.ne(null)) { <next pattern> } else case12()

WIP: https://github.com/scala/scala/compare/2.13.x...retronym:topic/shallower-patmat-ast?expand=1

sjrd commented 5 years ago

The Scala.js backend won't be happy at all if you do that. It's very sensitive to the shapes of trees produced by the pattern matcher. The function genTranslatedMatch in GenJSCode would have to be rewritten.

retronym commented 5 years ago

Thanks for the heads up, @sjrd. We'll be make sure to allow time for that re-write, or otherwise, we can make the status quo available under a migration option of some sort.

lrytz commented 5 years ago

Just saw the SOE on the most recent merge commit in 2.13.x, which includes the -Xss1m

SethTisue commented 5 years ago

gah, not sure what to do here, earliest I could look at it or think about it is next week

milessabin commented 5 years ago

Is there any reason for not being a bit more generous with heap and stack? I use 3g and 2m respectively for shapeless.

lrytz commented 5 years ago

I guess we'd like to understand why this started happening recently and keep some pressure to do something about it. But the cost of having spurious failures over and over again is probably a bit too high. So I'm happy to bump to 2m.

SethTisue commented 5 years ago

SGTM. we could leave this ticket open unless or until we decide do something else

retronym commented 5 years ago

Want to try https://github.com/scala/scala/pull/7375 first?

SethTisue commented 5 years ago

I don't recall seeing this happen recently