Skip to content

Make V8 stancjs in .OnLoad() - #946

Merged
hsbadr merged 5 commits into
experimentalfrom
fix/v8-mem
Jun 17, 2021
Merged

Make V8 stancjs in .OnLoad()#946
hsbadr merged 5 commits into
experimentalfrom
fix/v8-mem

Conversation

@SteveBronder

Copy link
Copy Markdown
Contributor

Summary:

This hopefully fixes #945 and rstudio/rstudio#9282 by moving the stancjs setup logic to .OnLoad().

I know how busy @jeroen but Jeroen if you would have time to take a peek at this it would be very helpful. Essentially I think the problem is that v8 is a global object and calling ctx$source(stanc_file) was causing memory to continuously be used. Calling gc() wouldn't help with this since v8 is a global.

I also removed allow_optimizations from stanc and friends. Those are super experimental and can potentially produce incorrect C++ and are not ready to be exposed to users. My big concern here is that users are going to see "allow optimizations" and go "well of course yes" without actually knowing they are experimental / dangerous. We are also working right now to have optimization levels like 1,2,3 etc. so that interface would change anyway.

Intended Effect:

Fixes the above memory leaks

How to Verify:

I'm actually not sure how to verify this I was just taking an example model and doing

for (i in 1:500) {
  stanc_builder("../test_mod.stan")
}

and checking the memory usage via htop to see if it kept going up.

Side Effects:

Documentation:

Reviewer Suggestions:

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Steve Bronder

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@jeroen

jeroen commented Jun 16, 2021

Copy link
Copy Markdown
Contributor

I think the line with stanc_ctx <- V8::v8() is redundant, as you already create stanc_ctx in your onLoad().

Here is a minimal example: https://github.com/ropensci/jsonld/blob/master/R/onload.R

Otherwise it looks good.

Comment thread rstan/rstan/R/zzz.R Outdated
Comment thread rstan/rstan/R/zzz.R Outdated
Comment thread rstan/rstan/R/zzz.R
Comment thread rstan/rstan/R/zzz.R Outdated
Comment thread rstan/rstan/R/zzz.R Outdated
Comment thread rstan/rstan/R/zzz.R Outdated
@hsbadr

hsbadr commented Jun 17, 2021

Copy link
Copy Markdown
Member

LGTM. Thanks @SteveBronder!

@hsbadr
hsbadr merged commit 9a2e5e8 into experimental Jun 17, 2021
@rok-cesnovar
rok-cesnovar deleted the fix/v8-mem branch November 28, 2021 12:50
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