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

memory leak in cuecontext.CompileString? #1424

Closed
abngoal opened this issue Dec 10, 2021 · 9 comments
Closed

memory leak in cuecontext.CompileString? #1424

abngoal opened this issue Dec 10, 2021 · 9 comments

Comments

@abngoal
Copy link

abngoal commented Dec 10, 2021

package main

import (
	"log"
	"time"

	"cuelang.org/go/cue/cuecontext"
)

func main() {
	for {
		parse()
		time.Sleep(10 * time.Millisecond)
	}
}

func parse() {
	var err error
	ctx := cuecontext.New()

	value := ctx.CompileString(schema)
	if err = value.Err(); err != nil {
		log.Fatalln(err)
	}
}

const schema = `
#testa: int
#testb: int
`

image

i use the goapi in my server In the same way as above. However, I found that frequent calls to 'CompileString' led to uncontrolled memory.
Maybe I'm using it wrong. Who can tell me how to use the api correctly

@abngoal abngoal added NeedsInvestigation Triage Requires triage/attention labels Dec 10, 2021
@eonpatapon
Copy link
Contributor

Maybe reuse the same cuecontext ? You don't need to create a new one everytime.

@verdverm
Copy link

verdverm commented Dec 10, 2021

I don't believe this is a memory leak as much as the shared index building up over time and not releasing memory. The evaluator currently has some global stuff which is why you cannot perform certain operations without locking and the memory footprint will increase indefinitely.

At this point, you will have to do hacky things for long running CUE processes. (i.e. gracefully restart the server every so often and run enough copies to keep the API available)

If you are using the same schema on every call, it will be beneficial to parse it once and reuse it in subsequent calls.

note, this is expected to change over time. there are other issues tracking these features

@abngoal
Copy link
Author

abngoal commented Dec 10, 2021

@verdverm thanks for your answer. I'm going to try to solve this problem by calling cue directly, although the performance will be worse

@abngoal
Copy link
Author

abngoal commented Dec 10, 2021

Maybe reuse the same cuecontext ? You don't need to create a new one everytime.

I have tried this method, and the problem is still there but not as severe as before

@kentalee
Copy link

kentalee commented Dec 11, 2021

cuecontext.New() call internal/core/runtime/Runtime.New(), which create new runtime object with global sharedIndex @ internal/core/runtime/imports.go :

// sharedIndex is used for indexing builtins and any other labels common to
// all instances.
var sharedIndex = newIndex()

instances built by runtime are used by the global variable sharedIndex, and will not be freed after cueContext delete by gc

Perhaps similar to #1418 #1414

@kentalee
Copy link

maybe this fix could help (on your own risk): https://github.com/lipence/cue/commit/6ed69100ebd62509577826657536172ab46cf257

replace cuelang.org/go => github.com/lipence/cue v0.4.1-beta.6a

@abngoal
Copy link
Author

abngoal commented Dec 13, 2021

@kentalee

replace cuelang.org/go => github.com/lipence/cue v0.4.1-beta.6a

it works!

@verdverm
Copy link

verdverm commented Dec 16, 2021

I kind of think #1418 is better in that it raises the idea of letting users configure the index scope.

  • I may want to use a single, shared index, such that it doesn't matter which context values are created with, they can still be unified and compared
  • I may want each index to be isolated and independent, for example a long running API service
  • I may want something in the middle, where I create a global context containing schemas, and then create a new context as a clone of this global, so that I can isolate independent request while still benefiting from only parsing the schemas once

I don't think CUE can necessarily infer when I may be done with a value, so we probably need a way for users to control this due to the memory pressure the current behavior creates.

@mpvl
Copy link
Member

mpvl commented Feb 19, 2022

@kentalee: yes, that is indeed the problem. There was some reason why this wasn't done yet exactly as you said.

I think part of the issue was that builtins could not yet be treated the same way, even though this is not possible yet and then a proper fix for the packages, at least, dropped off the radar.

@eonpatapon: in this case, to avoid growing memory, one actually _should _create a new Context on each iteration. The problem is indeed as @kentalee identified.

cueckoo pushed a commit that referenced this issue Feb 19, 2022
This was always supposed to be this way, but dropped off
the radar as a thing to do.

Fixes #1424
Fixes #1418
Issue #1414

Signed-off-by: Marcel van Lohuizen <mpvl@golang.org>

Change-Id: I25b486d2ea1c6009b00732a113af29bd485f9df5
@mpvl mpvl added NeedsFix and removed NeedsInvestigation Triage Requires triage/attention labels Feb 19, 2022
@mpvl mpvl added this to the v0.4.3 bug fixes milestone Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants