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 subscriber-collection.js in case of complex bindings #585

Closed
reinholdk opened this Issue Apr 3, 2017 · 2 comments

Comments

Projects
None yet
5 participants
@reinholdk

reinholdk commented Apr 3, 2017

I'm submitting a bug report

  • Library Version:
    1.2.1

Please tell us about your environment:

  • Operating System:
    Windows 10

  • Node Version:
    6.9.1

  • NPM Version:
    4.0.5

  • JSPM OR Webpack AND Version
    JSPM 0.16.46

  • Browser:
    Chrome 57

  • Language:
    ESNext

Current behavior:
When using complex binding expressions inside of a repeat, old subscribers do net get cleaned-up on itemsChanged.

Expected/desired behavior:
The example is a list of 100 items that can be filtered by a search box:
https://gist.run/?id=ed14778cdbeccb3888c6ee0c16b8fc71
Set a breakpoint to:


and enter a search value, e.g. '1' into the search box.
Each time you change the search value the repeat items change and the size of this._callablesRest grows (~200, ~300, ~500, ...).

  • What is the expected behavior?
    Outdated subscriptions are not retained by the framework since they may keep a large amout of memory from being freed by the GC.

  • What is the motivation / use case for changing the behavior?
    Allowing to implementing re-usable UI components which require more complex binding expressions, e.g. a data driven list with search and "add new row" capabilities.

@AshleyGrant

This comment has been minimized.

Show comment
Hide comment
@AshleyGrant

AshleyGrant Aug 16, 2017

Member

@jdanyow have you had a chance to look at this? It is related to aurelia/router#456 this is affecting a client of mine.

Thanks!

Member

AshleyGrant commented Aug 16, 2017

@jdanyow have you had a chance to look at this? It is related to aurelia/router#456 this is affecting a client of mine.

Thanks!

@bigopon

This comment has been minimized.

Show comment
Hide comment
@bigopon

bigopon Sep 10, 2017

Member

@jdanyow I don't think complex binding is the root cause and as hinted by @reinholdk , it is because the way rest works currently in subscriberCollection/removeSubscriber line 48 in subscriber-collection.js

if (!rest
  || !rest.length
  || (index = rest.indexOf(context)) === -1 // Issue here, context in repeat are often the same
  || this._callablesRest[index] !== callable) // thus this line will be wrong, as the index is of something else

Potential PR: break the if into smaller chunk to handle it correctly

  let rest = this._contextsRest;
  let callablesRest = this._callablesRest;
  let index;
  // this is normal
  if (!rest || !rest.length) {
    return false;
  }
  // we can't rely on context, since it's repeated when it comes to rest
  // So we have to use the callable
  if ((index = callablesRest.indexOf(callable)) === -1) {
    return false;
  }
  // Still it's not reliable enough, same callable may have both
  // sourceContext and targetContext
  // Need to check one more round
  if (rest[index] !== context) {
    let len = callablesRest.length;
    while (len > index) {
      if (rest[index] === context) {
        break;
      }
      index++;
    }
    if (index === len) {
      return false;
    }
  }
  // If every condition passed, splice:
  rest.splice(index, 1);
  callablesRest.splice(index, 1);
  return true;

Related:

Temporary fix: Replace the whole removeSubscriber in aurelia-binding/src/subscriber-collection with following function (expand detail). It's quite impossible to patch in Gist so can't put it there.

function removeSubscriber(context, callable) {
  if (this._context0 === context && this._callable0 === callable) {
    this._context0 = null;
    this._callable0 = null;
    return true;
  }
  if (this._context1 === context && this._callable1 === callable) {
    this._context1 = null;
    this._callable1 = null;
    return true;
  }
  if (this._context2 === context && this._callable2 === callable) {
    this._context2 = null;
    this._callable2 = null;
    return true;
  }
  let rest = this._contextsRest;
  let callablesRest = this._callablesRest;
  let index;
  if (!rest || !rest.length) {
    return false;
  }
  if ((index = callablesRest.indexOf(callable)) === -1) {
    return false;
  }
  if (rest[index] !== context) {
    let len = callablesRest.length;
    while (len > index) {
      if (rest[index] === context) {
        break;
      }
      index++;
    }
    if (index === len) {
      return false;
    }
  }
  rest.splice(index, 1);
  callablesRest.splice(index, 1);
  return true;
}


I have pulled down the Gist, tested locally with the changes above and the issue is gone.

@jdanyow @EisenbergEffect I dont know if this will have impact on benchmark tests, so I haven't created a PR. What do you think ?

Member

bigopon commented Sep 10, 2017

@jdanyow I don't think complex binding is the root cause and as hinted by @reinholdk , it is because the way rest works currently in subscriberCollection/removeSubscriber line 48 in subscriber-collection.js

if (!rest
  || !rest.length
  || (index = rest.indexOf(context)) === -1 // Issue here, context in repeat are often the same
  || this._callablesRest[index] !== callable) // thus this line will be wrong, as the index is of something else

Potential PR: break the if into smaller chunk to handle it correctly

  let rest = this._contextsRest;
  let callablesRest = this._callablesRest;
  let index;
  // this is normal
  if (!rest || !rest.length) {
    return false;
  }
  // we can't rely on context, since it's repeated when it comes to rest
  // So we have to use the callable
  if ((index = callablesRest.indexOf(callable)) === -1) {
    return false;
  }
  // Still it's not reliable enough, same callable may have both
  // sourceContext and targetContext
  // Need to check one more round
  if (rest[index] !== context) {
    let len = callablesRest.length;
    while (len > index) {
      if (rest[index] === context) {
        break;
      }
      index++;
    }
    if (index === len) {
      return false;
    }
  }
  // If every condition passed, splice:
  rest.splice(index, 1);
  callablesRest.splice(index, 1);
  return true;

Related:

Temporary fix: Replace the whole removeSubscriber in aurelia-binding/src/subscriber-collection with following function (expand detail). It's quite impossible to patch in Gist so can't put it there.

function removeSubscriber(context, callable) {
  if (this._context0 === context && this._callable0 === callable) {
    this._context0 = null;
    this._callable0 = null;
    return true;
  }
  if (this._context1 === context && this._callable1 === callable) {
    this._context1 = null;
    this._callable1 = null;
    return true;
  }
  if (this._context2 === context && this._callable2 === callable) {
    this._context2 = null;
    this._callable2 = null;
    return true;
  }
  let rest = this._contextsRest;
  let callablesRest = this._callablesRest;
  let index;
  if (!rest || !rest.length) {
    return false;
  }
  if ((index = callablesRest.indexOf(callable)) === -1) {
    return false;
  }
  if (rest[index] !== context) {
    let len = callablesRest.length;
    while (len > index) {
      if (rest[index] === context) {
        break;
      }
      index++;
    }
    if (index === len) {
      return false;
    }
  }
  rest.splice(index, 1);
  callablesRest.splice(index, 1);
  return true;
}


I have pulled down the Gist, tested locally with the changes above and the issue is gone.

@jdanyow @EisenbergEffect I dont know if this will have impact on benchmark tests, so I haven't created a PR. What do you think ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment