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

Possible memory leak ? #49

Open
vlarmet opened this issue Oct 9, 2020 · 9 comments
Open

Possible memory leak ? #49

vlarmet opened this issue Oct 9, 2020 · 9 comments

Comments

@vlarmet
Copy link

vlarmet commented Oct 9, 2020

Hi,

First of all, thank you for this great package !
I want to use leafgl to display a large shapefile (~35000 polygons) in Shiny.
The user can choose a variable to map and the map is updated through leafletProxy.
However, I notice that memory usage increase each time the user choose a new variable, here is a reproductible example.

library(shiny)
library(leaflet)
library(leafgl)
library(raster)
library(sf)

# Create 50000 pixels raster
r <- raster(extent(matrix( c(-90, -50, 90,  50), nrow=2)), nrow=500, ncol=100, 
            crs = "+proj=longlat +datum=WGS84 +no_defs +ellps=WGS84 +towgs84=0,0,0") 

Extent <- extent(r)

# Convert to sf shapefile
shp <- rasterToPolygons(r)
shp <- st_as_sf(shp)

## UI ##########
ui <- fluidPage(
  actionButton("go","go"),
  leafglOutput("map")
)

## SERVER ##########
server <- function(input, output) {
  output$map <- renderLeaflet({
    leaflet()  %>% 
      addTiles() %>% 
      fitBounds(lng1 = Extent[1],lat1 = Extent[3], lng2 = Extent[2], lat2 = Extent[4])
  })
  
  observeEvent(input$go,{
    random <- sample(1:100,50000,replace = T)
    pal = colorQuantile(rev(viridis::viridis(10)), random , n = 10)
    
    leafletProxy("map") %>%
      leafgl::removeGlPolygons(layerId = "polygons") %>%
      leafgl::addGlPolygons(layerId = "polygons",data = shp, color = ~pal(random))
  })
}

shinyApp(ui, server)

Each time I click go button, memory increase by about 100Mo.

@trafficonese
Copy link
Collaborator

trafficonese commented Oct 9, 2020

I think that problem might come from the upstream repo, which is not fully removing the glify instances. (see this issue)
In the browser console you can see that L.glify.instances increases with every click on go.

@vlarmet
Copy link
Author

vlarmet commented Oct 9, 2020

Thanks for the reply.
So there is no way to fix that issue at the moment ?

@trafficonese
Copy link
Collaborator

If those are the only polygons you are plotting, you could remove the instances yourself with shinyjs.

In the ui you need to include useShinyjs() and after the leafletProxy call you can run runjs("L.glify.Shapes.instances.splice(0, 1);"). Maybe that would work as a workaround?

@tim-salabim
Copy link
Member

Thanks @trafficonese for chiming in here. I have sent you an invite with write access to this repo. I figure you're contributing so much here, you might as well have write access...

@vlarmet
Copy link
Author

vlarmet commented Oct 9, 2020

Wow thank you very much @trafficonese !
It work like a charm.

edit : here is the solution

library(shiny)
library(leaflet)
library(leafgl)
library(raster)
library(sf)
library(shinyjs)
library(jsonify)
library(geojsonsf)

# Create 50000 pixels raster
r <- raster(extent(matrix( c(-90, -50, 90,  50), nrow=2)), nrow=500, ncol=100, 
            crs = "+proj=longlat +datum=WGS84 +no_defs +ellps=WGS84 +towgs84=0,0,0") 

Extent <- extent(r)

# Convert to sf shapefile
shp <- rasterToPolygons(r)
shp <- st_as_sf(shp)

## UI ##########
ui <- fluidPage(
  useShinyjs(),
  actionButton("go","go"),
  leafglOutput("map")
)

## SERVER ##########
server <- function(input, output) {
  output$map <- renderLeaflet({
    leaflet()  %>% 
      addTiles() %>% 
      fitBounds(lng1 = Extent[1],lat1 = Extent[3], lng2 = Extent[2], lat2 = Extent[4])
  })
  
  observeEvent(input$go,{
    random <- sample(1:100,50000,replace = T)
    pal = colorQuantile(rev(viridis::viridis(10)), random , n = 10)
    
    runjs("L.glify.Shapes.instances.splice(0, 1);")

    leafletProxy("map") %>%
      leafgl::removeGlPolygons(layerId = "polygons") %>%
      leafgl::addGlPolygons(layerId = "polygons",data = shp, color = ~pal(random))
  })
}

shinyApp(ui, server)

@vlarmet vlarmet closed this as completed Oct 9, 2020
@trafficonese
Copy link
Collaborator

I would keep this issue open as this is more of a dirty hack than a proper fix. If multiple layers are included and the one to be removed is not the first in the array, the solution will not work. So, I think this should actually be handled in the underlying library.

Thanks @tim-salabim, I really appreciate it!

@trafficonese trafficonese reopened this Oct 12, 2020
@h-a-graham
Copy link

Hey all,
I'm coming up against this issue using addGlPolylines() and varSelectInput() - If Ichange the column input to the polylines more than ~3 times the app runs out of memory. I have tried the suggested workaround here along with the following (and some variations around it):

runjs("L.glify.Lines.instances.splice(0, 1);")

Neither seem to solve the problem - just wondering if there is an updated workaround or alternative that I can consider?
many thanks,
Hugh

@trafficonese
Copy link
Collaborator

The issue is documented here robertleeplummerjr/Leaflet.glify#129 (comment)

I just pushed a PR to the upstream repo to address it. robertleeplummerjr/Leaflet.glify#153 (comment)

In my opinion this is currently the biggest issue for using the library in an interactive context (e.g. Shiny), where one needs to remove/update a layer many times. Otherwise we just add new WebGL contexts and at some point the browser will tell us that the oldest WebGL contexts are lost and the old layers are gone..

❗ When this gets merged and we add it here, the old workaround using runjs("L.glify.Lines.instances.splice(0, 1);") needs to get removed.

Also @tim-salabim, since you updated to the new Leaflet.glify version (which btw. is not registered in Github as a release ❓ ), we should expose the methods update/remove/insert for the same reasons.

I will try to find some time at the end of this week to open a new PR, if thats ok with you?

@tim-salabim
Copy link
Member

Thanks @trafficonese ! That's fine. I'll take a look at your PR as soon as it's ready

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

No branches or pull requests

4 participants