Skip to content

Fix memory leak in GDScript during infinnity loops with yields#19828

Merged
mhilbrunner merged 1 commit into
godotengine:masterfrom
jkb0o:fix_yield_leak
Jul 3, 2018
Merged

Fix memory leak in GDScript during infinnity loops with yields#19828
mhilbrunner merged 1 commit into
godotengine:masterfrom
jkb0o:fix_yield_leak

Conversation

@jkb0o
Copy link
Copy Markdown
Contributor

@jkb0o jkb0o commented Jun 28, 2018

Fixes #19823 introduced in #17291

The code below passes assertions

extends Node

var iterations = 5;

func _ready():
	print("start single_yield()")
	assert(yield(single_yield(), "completed") == 1)
	print("done single_yield()")
	print("start three_yields()")
	assert(yield(three_yields(), "completed") == 3)
	print("done three_yields()")
	var num_objects = Performance.get_monitor(Performance.OBJECT_COUNT) - 2
	while iterations:
		yield(get_tree(), "idle_frame")
		yield(get_tree(), "idle_frame")
		yield(get_tree(), "idle_frame")
		var current_objects = Performance.get_monitor(Performance.OBJECT_COUNT)
		print("yield 3 idle frames, current objects == ", current_objects, ", awaits ", num_objects, " objects")
		assert(num_objects == current_objects)
		iterations -= 1
	
func single_yield():
	var i = 0
	print("  single_yield: -> yield1")
	yield(get_tree(), "idle_frame")
	print("  single_yield: -> yield1 ->")
	i += 1
	return i
	
func three_yields():
	var i = 0
	print(" three_yields: -> yield1")
	yield(get_tree(), "idle_frame")
	
	print(" three_yields: -> yield1 -> yield2")
	i += 1

	yield(get_tree(), "idle_frame")
	
	print(" three_yields: -> yield1 -> yield2 -> yield3")	
	i += 1

	yield(get_tree(), "idle_frame")
	
	i += 1
	print(" three_yields: -> yield1 -> yield2 -> yield3 ->")	
	return i

and produce output

start single_yield()
  single_yield: -> yield1
  single_yield: -> yield1 ->
done single_yield()
start three_yields()
 three_yields: -> yield1
 three_yields: -> yield1 -> yield2
 three_yields: -> yield1 -> yield2 -> yield3
 three_yields: -> yield1 -> yield2 -> yield3 ->
done three_yields()
yield 3 idle frames, current objects == 837, awaits 837 objects
yield 3 idle frames, current objects == 837, awaits 837 objects
yield 3 idle frames, current objects == 837, awaits 837 objects
yield 3 idle frames, current objects == 837, awaits 837 objects
yield 3 idle frames, current objects == 837, awaits 837 objects

Also I'm attaching example project witch fails with 3.0.3+ and passes with current pr.
godot_yield_leak.zip

CC @Warlaan

@jkb0o jkb0o requested review from bojidar-bg, reduz and vnen as code owners June 28, 2018 15:54
Comment thread modules/gdscript/gdscript_function.cpp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't put condition and code block inline for ifs, it makes it hard to read what is the condition and what is the statement that will be executed. We tolerate it for simple if (cond) return false;, but even then I'd advise not to use it.

Copy link
Copy Markdown
Contributor Author

@jkb0o jkb0o Jun 28, 2018

Choose a reason for hiding this comment

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

Ah, makes sense. Fixed.

@puppetmaster-
Copy link
Copy Markdown

this script should be included in the Godot test project 😄

@jkb0o
Copy link
Copy Markdown
Contributor Author

jkb0o commented Jun 29, 2018

It actually is. But it is also here for those lazy and trustful persons like me (=

@mhilbrunner mhilbrunner merged commit aad4759 into godotengine:master Jul 3, 2018
williamd1k0 added a commit to williamd1k0/godot that referenced this pull request Jul 27, 2018
@akien-mga
Copy link
Copy Markdown
Member

Cherry-picked for 3.0.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak when yielding (regression from #17291)

4 participants