-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
Fix memory leak #442
Fix memory leak #442
Conversation
The test failure looks new. Can it be related to the change? |
|
||
var contextKey key | ||
var contextKey = contextKeyStruct{} | ||
var handleKey = handleKeyStruct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed contextKey
to follow Go best-practices and ensure uniqueness during runtime.
@@ -480,7 +490,6 @@ func maybeCloseContext(fc *FrankenPHPContext) { | |||
//export go_execute_script | |||
func go_execute_script(rh unsafe.Pointer) { | |||
handle := cgo.Handle(rh) | |||
defer handle.Delete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now deleted by the handle list.
This is now ready for review @dunglas |
You rock 😍 I'll review ASAP! Don't worry about the CI failure, I'll take care of it. |
Looks like there is still a leak in cgi-mode, trying to figure out the best way to fix it. |
argc := C.int(len(args)) | ||
argv := make([]*C.char, argc) | ||
for i, arg := range args { | ||
argv[i] = C.CString(arg) | ||
defer C.free(unsafe.Pointer(argv[i])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stumbled across this one by accident. If my IDE warning is correct and I understand correctly, defer
in a for-loop will actually just call defer C.free(unsafe.Pointer(argv["last-i"]))
i times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭 good one, the typical for loop bug.
It should be g2g if you're happy with it. The best way to see the memory leak (or lack thereof) is to run k6 with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing hunt and fix!
It appears we aren't deleting ALL the created handles. This PR introduces a "smartpointer.go" file which can manage handles (and pointers, though it isn't used in this PR -- can remove the pointer code) without having to keep track of them manually. I like it because it frees up the cognitive load when trying to ensure everything is actually freed, without having to trace through the entire program's execution to figure out whether or not it is freed.
To illustrate the difference, here are screen shots after 1:15 of running a load-test on a 2gb container (pay attention to memory usage).
fixes #366
^ before this change
^ after this change (~7k RPS!)