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
Use fastmap as backing store for Map class #2429
Conversation
c4ffb13
to
da5811e
Compare
In another test, I disabled all reactlog-related stuff in reactives.R, and I found that the test cases leaked the following amounts of memory: 76160, 0, 0, 0, 0, and 0 bytes. So with fastmap and no reactlog code, the only problematic case is the one where a This change also completely eliminated leakage from @alandipert's test app at #2321 (comment). @schloerke I think we'll want to reinstate an updated version of #2376 to eliminate the reactlog-related leaks. |
0b74294
to
228a1a5
Compare
R/reactives.R
Outdated
all.names=TRUE | ||
) | ||
dep.keys <- .dependents$keys() | ||
dep.keys <- dep.keys[grepl(paste('^\\Q', key, ':', '\\E', '\\d+$', sep=''), dep.keys)] |
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 could be grep(value=TRUE)
I think?
TODO: Test |
There seems to be a change in the order that reactives/observers execute. Here's a test app: library(shiny)
ui <- fluidPage(
actionButton("go", "Go"),
verbatimTextOutput("txt"),
verbatimTextOutput("log")
)
server <- function(input, output, session) {
log <- reactiveVal(character(0))
log_append <- function(...) {
isolate(log(c(
log(),
paste(..., sep = "")
)))
}
observe({
input$go
log_append(input$go, ": observe")
})
output$txt <- renderText({
log_append(input$go, ": renderText")
paste("Number of button presses:", input$go)
})
output$log <- renderText({
paste0(
log(),
collapse = "\n"
)
})
}
shinyApp(ui, server) With shiny 1.3.2 in a new R session, the output looks like this after clicking 5 times: When running the app a second time (still with 1.3.2), the order is consistent: With this branch, the order switches every time the button is clicked: |
This is still a work in progress. It replaces the environment-based
Map
class with one based on the fastmap package. The reason for this is that environment-based maps can leak memory, whereas fastmap does not.This partially fixes #2423. Before the change, the six test cases from that issue leak the following amounts of memory: 76528, 2824, 1480, 2352, 1168, and 1168 bytes (note that these numbers came out somewhat differently in this test run compared to when I tested it in #2423). After this PR, the test cases leak 76520, 256, 192, 512, 64, and 64 bytes.
Update: This should be good to merge now.