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

Prevent stack overflow in Profile #31893

Merged
merged 2 commits into from
May 2, 2019
Merged

Prevent stack overflow in Profile #31893

merged 2 commits into from
May 2, 2019

Conversation

Keno
Copy link
Member

@Keno Keno commented May 1, 2019

As a follow up to #31693, this fixes the other place in Profile
that recurses over Profile data, causing stack overflows. This
should fix a bunch of the recent intermittent CI faults on linux32.

As a follow up to #31693, this fixes the other place in Profile
that recurses over Profile data, causing stack overflows. This
should fix a bunch of the recent intermittent CI faults on linux32.
vtjnash
vtjnash previously requested changes May 1, 2019
println(io, isempty(str) ? "$count unknown stackframe" : str)
noisefloor_down = fmt.noisefloor > 0 ? floor(Int, fmt.noisefloor * sqrt(count)) : 0
tree(io, down, level + 1, cols, fmt, noisefloor_down)
worklist = Tuple{StackFrameTree, Int, Int, Union{String, Nothing}}[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
worklist = Tuple{StackFrameTree, Int, Int, Union{String, Nothing}}[
worklist = [(bt, level, noisefloor, "")]

No sense in reducing performance and introducing pointless complexity.

But I don't think this translation is correct: it doesn't seem expected that str would suddenly get appended to the worklist, when it wasn't there before the translation. It looks like this would print all level 0 items, followed by all level 1 items, followed by all level 2 items, and so on. (indeed, running this PR on @profile peakflops(), that's exactly the behavior that I see happen)

@vtjnash vtjnash dismissed their stale review May 1, 2019 19:05

pushed commit to push items in order

@Keno Keno merged commit 1707e13 into master May 2, 2019
@Keno
Copy link
Member Author

Keno commented May 2, 2019

Thanks for the fixup.

@StefanKarpinski StefanKarpinski deleted the kf/profilestkovf branch May 2, 2019 12:47
@JeffBezanson JeffBezanson mentioned this pull request May 15, 2019
58 tasks
KristofferC pushed a commit that referenced this pull request May 15, 2019
As a follow up to #31693, this fixes the other place in Profile
that recurses over Profile data, causing stack overflows. This
should fix a bunch of the recent intermittent CI faults on linux32.

(cherry picked from commit 1707e13)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants