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

use data structure Map with cleanup #223

Closed

Conversation

hopeful2
Copy link
Contributor

This PR uses data structure Map and takes care of cleaning up. It is actually easier to have the clean up code in this library. It also addresses the GC issue in older browsers which use a Map anyway.

I have tested the minified Shy version in browser manually and made sure bind/unbind work as expected.

As per mavoweb/mavo issue #417, we need to loop through elements, unbind events and manually delete elements in order to break circular reference. A data structure of Map allows us to do that.

@hopeful2
Copy link
Contributor Author

When you have a chance, could you review and guide the next steps? A destroy method of Mavo instance depends on this library for proper cleaning up. If possible, once a direction is decided, (optional Map vs always Map ), the latest version of of Bliss can be included in a development branch of Mavo, so more developers can get involved in testing. Thanks

@LeaVerou
Copy link
Owner

Hi there,

I was holding off merging this until the other discussion resolves!

@hopeful2
Copy link
Contributor Author

hopeful2 commented Jan 1, 2019

greetings!

Have you found a better way to handle the circular reference and memory issues in Bliss/Mavo? the destroy method of Mavo depends on the ability to break circular reference manually, and thus further changes in Mavo depend on whether this PR is merged. Please review and advise.

@LeaVerou
Copy link
Owner

LeaVerou commented Jan 7, 2019

Hi @hopeful2, so sorry for the huge delay, holiday season! Happy new year!

From a quick look, it looks like eventCount is never reduced, is that intentional?

I may be forgetting part of our discussion since it's been a while, but are you 100% sure removing listeners is needed for garbage collection? Could it be that something else is causing the memory issues you are observing? AFAIK the whole point of not being able to retrieve listeners in browsers is gc, and since WeakMap itself does not retain any reference to the element, if having listeners around prevents gc, it sounds like browsers are seriously buggy.

I'm not opposed to this change, I'm just very nervous about it because it makes gc worse in cases where the library user does not manually unbind their listeners when needed, and that's the majority of cases...

@hopeful2
Copy link
Contributor Author

hopeful2 commented Jan 7, 2019

Hi @LeaVerou , no worries. I understand it is a holiday season. Happy new year to you too. You are the most efficient person I know in responding to issues and merging PR's while maintaining multiple projects.

Yes the behaviour of eventCount is intentional. It is increased by 1 only if an event type (eg click) has at least one callback handler. It serves as a boolean flag and keeps track of whether there are event types left with non-empty array of event handlers after mutating that array. If there is no event type left, we will remove the element from the map to allow GC.

I am 100% sure event listeners have to be removed for GC to work, at least on the version of Chrome/OSX I am using. When a browser registers an event listener callback function, it stores the element in the callback context. When the element is removed from the DOM, the callback context still holds reference to the detached element. In the numerous memory timeline recordings, I have observed the detached elements were still there after forced garbage collection. Once the event listeners were removed, the detached elements were gone.

I am not sure whether this is a browser bug or intended behaviour. When removing elements permanently, developers are encouraged to clean up (unbind) event listeners manually regardless. For example, jQuery removes event listeners added through jQuery when an element is removed.

This type of memory leak may not be an issue if the page is refreshed frequently, or the elements remain in the DOM. It is only an issue when elements are frequently added and removed from the DOM, such as an editor, or a long running app.

A Map allows us flexibility to loop and remove listeners and elements when needed. Please consider.

@hopeful2
Copy link
Contributor Author

hopeful2 commented Jan 8, 2019

Upon second thought, I think this may not be a browser bug. The root cause is mostly likely circular reference in the callback function (closure) of event listener, which somehow the browser fails to resolve. The callback function often contains reference to a Mavo instance or Mavo node instance, which points to elements. It will take much much more time and effort to refactor and debug the data structure in Mavo. This PR is an easier fix.

Even if some users of Bliss do not use Mavo, they may have circular reference in their callback function and encounter similar issues. They will benefit from this PR as long as they manage event listeners using Bliss.bind() and Bliss.unbind() methods.

Your thoughts?

@LeaVerou
Copy link
Owner

LeaVerou commented Jan 8, 2019

First off, the vast majority of Bliss users do not use Mavo, not some.

The problem is that most developers do not cleanup with unbind() afterwards, so this would break gc for them. If the issue is specific to Mavo, it should be addressed in Mavo. If it affects Bliss usage in general, it should be fixed in Bliss. But this is a serious breaking change, and we need to be sure we're doing the right thing.
Perhaps we can test if this truly hinders gc by creating a reduced testcase that doesn't use Mavo?

@hopeful2
Copy link
Contributor Author

hopeful2 commented Jan 8, 2019

I understand your concerns. Shall we use a fork of Bliss for Mavo then? or add a configuration option to use Map instead of WeakMap?

Actually if developers have circular reference in the callback function, there may not be any difference between a Map and a WeakMap. The developers have potential memory leak issues that they are not aware, or have not caused problems. The memory leak is only an issue for editor app or long running app with frequent adding/removing elements.

I have observed similar memory leak pattern when an object is bound to a callback function of event listeners, and the object contains reference to the element. Removing the reference to the object and removing the element from DOM resulted in detached elements. After the event listener is removed, the detached elements are gone. This has nothing to do with Bliss or WeakMap. Circular reference is probably an area where GC struggles with.

@hopeful2
Copy link
Contributor Author

hopeful2 commented Jan 8, 2019

In the local dev version, I have restored the data structure of Bliss.listeners to WeakMap and run more tests, and left event listeners for GC to handle. The new memory allocation recordings suggest that the circular reference has been resolved, and that the memory issue has gone. We can achieve the same result using a WeakMap for Bliss.listeners.

In the local dev version, I have used bind() to manage all event listeners, and used unbind() to remove event listeners when done. There are a few other changes in Mavo and our project code too. I am not sure exactly which change has resolved the issue -- that will take even more time to find out. Since the memory issue is related to Mavo or our project code, we can leave Bliss as is. I will close this PR then, and will prepare a new PR in Mavo.

thanks again for your wisdom and probing questions. I wish I had more time available for debugging.

ps. I accidentally included Bliss Full in the local dev version of Mavo but thought I was using Bliss Shy. Some (or even all) of previous recordings could have been done with Bliss Full which made the issue more complicated. Before submitting this PR, I tried WeakMap but couldn't resolve the issue.

In the new recordings today, I have made sure to use Bliss Shy, and have also allowed 30 seconds extra time for GC to work (after clicking the GC button in Chrome dev console). Hopefully this will help someone reading this conversation in the future trying to resolve a memory issue.

@hopeful2 hopeful2 closed this Jan 8, 2019
@LeaVerou
Copy link
Owner

LeaVerou commented Jan 8, 2019

That's so great to hear!! Thanks again for all your hard work looking into this! Looking forward to the Mavo PRs!

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.

2 participants