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

Minor frontend memory leaks due to unremoved LokiJS dynamic views #4248

Closed
jvilk opened this issue Aug 10, 2017 · 1 comment
Closed

Minor frontend memory leaks due to unremoved LokiJS dynamic views #4248

jvilk opened this issue Aug 10, 2017 · 1 comment

Comments

@jvilk
Copy link

jvilk commented Aug 10, 2017

Background

Loomio's angular frontend uses LokiJS Dynamic Views for a variety of tasks. Loomio created these dynamic views by calling addDynamicView on a collection. Internally, LokiJS appends new dynamic views to an array and returns it to Loomio. LokiJS expects that Loomio will later call removeDynamicView with the name of the dynamic view to remove the dynamic view instance from this internal array, but Loomio does not do this.

Since Loomio creates dynamic views every time a user navigates to new pages, this behavior results in a measurable memory leak.

Methodology

See #4247. For the base case here, I fixed the ment.io leaks.

Here are the fixes I made. They are likely not what you want to do to fix the behavior indefinitely, but they worked for the purpose of measuring the leak impact. It should be noted that LokiJS does not complain if you try to remove a dynamic view that does not exist.

core/services/thread_query_service.coffee: Fixing Loomio.records.discussions.collection

    createBaseView = (filters, queryType) ->
      Records.discussions.collection.removeDynamicView 'default'
      view = Records.discussions.collection.addDynamicView 'default'
      applyFilters(view, filters, queryType)
      view

    createGroupView = (group, filters, queryType, applyWhere) ->
      Records.discussions.collection.removeDynamicView group.name
      view = Records.discussions.collection.addDynamicView group.name
      view.applyFind({groupId: { $in: group.organisationIds() }})
      applyFilters(view, filters, queryType)
      view.applyWhere(applyWhere) if applyWhere?
      view

    createTimeframeView = (name, filters, queryType, from, to) ->
      today = moment().startOf 'day'
      Records.discussions.collection.removeDynamicView name
      view = Records.discussions.collection.addDynamicView name
      view.applyFind(lastActivityAt: { $gt: parseTimeOption(from) })
      view.applyFind(lastActivityAt: { $lt: parseTimeOption(to) })
      applyFilters(view, filters, queryType)
      view

core/models/poll_option_model.coffee: Fixing Loomio.records.stanceChoices.collection

    @dv: null

    relationships: ->
      @belongsTo 'poll'
      @dv = @hasMany 'stanceChoices'

    beforeRemove: ->
      _.each @stances(), (stance) -> stance.remove()
      Loomio.records.stanceChoices.collection.removeDynamicView @dv.name

core/models/comment_model.coffee: Fixing Loomio.records.versions.collection

    @dv: null

    relationships: ->
      @belongsTo 'author', from: 'users'
      @belongsTo 'discussion'
      @belongsTo 'parent', from: 'comments', by: 'parentId'
      @dv = @hasMany  'versions', sortBy: 'createdAt'

    beforeRemove: ->
      Loomio.records.versions.collection.removeDynamicView @dv.name

Results

Loomio leaks ~140 kilobytes (KB) every time the user navigates to a few pages.

loomio_dynamic_view_leaks

Suggestions

Refactor your code to appropriately clean up dynamic views when they are no longer needed.

@gdpelican
Copy link
Contributor

We've made some improvements in this area, and refactored ThreadQueryService to be a bit smarted about reusing existing views. Thanks for the report.

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

2 participants