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

Move element cache to the element[expando] to avoid cleanup and reduce code. #1734

Closed
mgol opened this Issue Oct 20, 2014 · 18 comments

Comments

Projects
None yet
3 participants
@mgol
Member

mgol commented Oct 20, 2014

Originally reported by john.david.dalton@… at: http://bugs.jquery.com/ticket/11570

Soo way back in 2005 Dean Edwards proposed an event system that slapped the listeners on the actual element. This allowed cache to be destroyed once the element was GC'ed.  http://dean.edwards.name/weblog/2005/10/add-event2/

Over the weekend Peter Michaux proposed a similar solution:  https://twitter.com/#!/petermichaux/status/189012245483757568  https://github.com/petermichaux/evento/blob/bd065a9b254a8d14b956ec438dc06c29622c3ef6/src/eventTarget.js#L165

I noticed that jQuery supports events on vanilla objects too so this change would align elements and objects.

The gist is that since jQuery is already adding an expando for a UID they can just use that property as the actual storage for the element.

Then all the code to recursively clean .removed'ed elements event/custom data could be removed as when the element is GC'ed that data is destroyed.

Issue reported for jQuery git

@mgol mgol added this to the 3.0.0 milestone Oct 20, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

I think this is worth investigating and experimenting. I'm going to grab it and work with @jdalton on implementing

Member

mgol commented Oct 20, 2014

Comment author: rwaldron

I think this is worth investigating and experimenting. I'm going to grab it and work with @jdalton on implementing

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jdalton

We may need to confirm the IE6 memory leak issue (or maybe not as that's really IE6 pre SP2) is handled (though if I remember correctly Dean Edwards 05 solution did *not* leak in IE).

Member

mgol commented Oct 20, 2014

Comment author: jdalton

We may need to confirm the IE6 memory leak issue (or maybe not as that's really IE6 pre SP2) is handled (though if I remember correctly Dean Edwards 05 solution did *not* leak in IE).

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

Once we exit the two-GC environment of oldIE we can implement this. However, we will still need to do the recursive crawl in at least some cases, to call any special event teardown hooks. We can probably optimize that significantly though, for example an expando flag that indicates whether there are any special events in use for each element.

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

Once we exit the two-GC environment of oldIE we can implement this. However, we will still need to do the recursive crawl in at least some cases, to call any special event teardown hooks. We can probably optimize that significantly though, for example an expando flag that indicates whether there are any special events in use for each element.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

@rwaldron, what should we do with this ticket?

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

@rwaldron, what should we do with this ticket?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

Fixed in 2.0 Data rewrite, which can be read here:  https://github.com/jquery/jquery/commits/master/src/data.js

Read backwards from 332a490

Member

mgol commented Oct 20, 2014

Comment author: rwaldron

Fixed in 2.0 Data rewrite, which can be read here:  https://github.com/jquery/jquery/commits/master/src/data.js

Read backwards from 332a490

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jdalton

It looks like it's still relying on data objects internally, that will leak if not discarded via an explicit call when the element is no longer needed. Using the element itself as the object which holds the data object would avoid this (the gist of this issue).

Member

mgol commented Oct 20, 2014

Comment author: jdalton

It looks like it's still relying on data objects internally, that will leak if not discarded via an explicit call when the element is no longer needed. Using the element itself as the object which holds the data object would avoid this (the gist of this issue).

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

The current design in 2.x is designed to support a smooth transition to a WeakMap as soon as they are available in at least 3 browsers (I picked that number arbitrarily).

Putting the data directly on the element would mean exposing jQuery's own interally used data (there are actually two sets of data for every object or element: user and private-interal), which we can't do. As long as user code does all of its DOM manipulation via jQuery, then data will be correctly cleaned up (removed).

Member

mgol commented Oct 20, 2014

Comment author: rwaldron

The current design in 2.x is designed to support a smooth transition to a WeakMap as soon as they are available in at least 3 browsers (I picked that number arbitrarily).

Putting the data directly on the element would mean exposing jQuery's own interally used data (there are actually two sets of data for every object or element: user and private-interal), which we can't do. As long as user code does all of its DOM manipulation via jQuery, then data will be correctly cleaned up (removed).

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jdalton

(there are actually two sets of data for every object or element: user and private-interal)

I know, the gist of this ticket was to suggest putting all element data (internal/user) on the element. That way there would be no need for the user to manually invoke a cleaning method. This would reduce the code needed for storing data and avoid memory leaks without burdening the dev with cleanup (which when overlooked, which is easy, can cause leaks that are not so easy to track down).

Member

mgol commented Oct 20, 2014

Comment author: jdalton

(there are actually two sets of data for every object or element: user and private-interal)

I know, the gist of this ticket was to suggest putting all element data (internal/user) on the element. That way there would be no need for the user to manually invoke a cleaning method. This would reduce the code needed for storing data and avoid memory leaks without burdening the dev with cleanup (which when overlooked, which is easy, can cause leaks that are not so easy to track down).

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

Replying to jdalton:

(there are actually two sets of data for every object or element: user and private-interal)

I know, the gist of this ticket was to suggest putting all element data (internal/user) on the element. That way there would be no need for the user to manually invoke a cleaning method. This would reduce the code needed for storing data and avoid memory leaks without burdening the dev with cleanup (which when overlooked, which is easy, can cause leaks that are not so easy to track down).

If user code can get at all of the jQuery-specific internal data, then we can't make any guarantees stability or reliability (http://bugs.jquery.com/ticket/11945,  http://forum.jquery.com/topic/what-s-the-rationale-of-not-exposing-object-s-events-handlers-collections). Trust me, I wish it was that easy :(

I always appreciate your input and feedback, but in this case we can't expose the jQuery-specific internal data on "owner" object.

Related, but not directly... I'm not sure what you mean by "need for the user to manually invoke a cleaning method"? This happens automatically with jQuery dom manip methods (where appropriate). The only time it won't happen is if user code uses DOM APIs directly, which is out of scope for jQuery.

Member

mgol commented Oct 20, 2014

Comment author: rwaldron

Replying to jdalton:

(there are actually two sets of data for every object or element: user and private-interal)

I know, the gist of this ticket was to suggest putting all element data (internal/user) on the element. That way there would be no need for the user to manually invoke a cleaning method. This would reduce the code needed for storing data and avoid memory leaks without burdening the dev with cleanup (which when overlooked, which is easy, can cause leaks that are not so easy to track down).

If user code can get at all of the jQuery-specific internal data, then we can't make any guarantees stability or reliability (http://bugs.jquery.com/ticket/11945,  http://forum.jquery.com/topic/what-s-the-rationale-of-not-exposing-object-s-events-handlers-collections). Trust me, I wish it was that easy :(

I always appreciate your input and feedback, but in this case we can't expose the jQuery-specific internal data on "owner" object.

Related, but not directly... I'm not sure what you mean by "need for the user to manually invoke a cleaning method"? This happens automatically with jQuery dom manip methods (where appropriate). The only time it won't happen is if user code uses DOM APIs directly, which is out of scope for jQuery.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

@jdalton, for the general case we still need to call our jQuery.cleanData() method in manipulation.js at least for situations like special events where we guarantee a teardown hook. Also consider our semantics for .remove() which say that when you remove elements from the document it removes both the events and data, whereas .detach() does not.

So we have to go through the loop part of the cleanData code regardless, but perhaps attaching the data to the element would let us skip some steps? If it made a significant performance difference I'd be interested in seeing an implementation, even if it "exposed" data to some extent. In doing perf testing on web apps and sites it's not uncommon to see cleanData high on the list, especially with MV* frameworks that update big DOM chunks.

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

@jdalton, for the general case we still need to call our jQuery.cleanData() method in manipulation.js at least for situations like special events where we guarantee a teardown hook. Also consider our semantics for .remove() which say that when you remove elements from the document it removes both the events and data, whereas .detach() does not.

So we have to go through the loop part of the cleanData code regardless, but perhaps attaching the data to the element would let us skip some steps? If it made a significant performance difference I'd be interested in seeing an implementation, even if it "exposed" data to some extent. In doing perf testing on web apps and sites it's not uncommon to see cleanData high on the list, especially with MV* frameworks that update big DOM chunks.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

We could "patchwelcome" this ticket, but the patch would have to pass all of the existing data tests as-is.

Member

mgol commented Oct 20, 2014

Comment author: rwaldron

We could "patchwelcome" this ticket, but the patch would have to pass all of the existing data tests as-is.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jdalton

Related, but not directly... I'm not sure what you mean by "need for the user to manually invoke a cleaning method"?

I was thinking of devs having to use .remove() and friends instead of say simply el.innerHTML=''. You're correct though if they were diligent and used only lib API it is managed.

If user code can get at all of the jQuery-specific internal data, then we can't make any guarantees stability or reliability

As for exposing data, pre 2.0, jQuery.cache existed so I'm not sure how that fits into the guarantees of stability or reliability.

As an aside, jQuery hasn't been a big user of hasOwnProperty checks so couldn't someone manipulate the Object.prototype and populate data values? Does that fit with stability/reliability guarantees?

Also I wanna +1 @dmethvin perf concern.

Member

mgol commented Oct 20, 2014

Comment author: jdalton

Related, but not directly... I'm not sure what you mean by "need for the user to manually invoke a cleaning method"?

I was thinking of devs having to use .remove() and friends instead of say simply el.innerHTML=''. You're correct though if they were diligent and used only lib API it is managed.

If user code can get at all of the jQuery-specific internal data, then we can't make any guarantees stability or reliability

As for exposing data, pre 2.0, jQuery.cache existed so I'm not sure how that fits into the guarantees of stability or reliability.

As an aside, jQuery hasn't been a big user of hasOwnProperty checks so couldn't someone manipulate the Object.prototype and populate data values? Does that fit with stability/reliability guarantees?

Also I wanna +1 @dmethvin perf concern.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: rwaldron

As for exposing data, pre 2.0, jQuery.cache existed so I'm not sure how that fits into the guarantees of stability or reliability.

In jQuery 2.x there is no jQuery.cache for this very reason :)

As an aside, jQuery hasn't been a big user of hasOwnProperty checks so couldn't someone manipulate the Object.prototype and populate data values? Does that fit with stability/reliability guarantees?

Sure, but that falls under the official Won't Fix :D  http://contribute.jquery.org/wont-fix/

Member

mgol commented Oct 20, 2014

Comment author: rwaldron

As for exposing data, pre 2.0, jQuery.cache existed so I'm not sure how that fits into the guarantees of stability or reliability.

In jQuery 2.x there is no jQuery.cache for this very reason :)

As an aside, jQuery hasn't been a big user of hasOwnProperty checks so couldn't someone manipulate the Object.prototype and populate data values? Does that fit with stability/reliability guarantees?

Sure, but that falls under the official Won't Fix :D  http://contribute.jquery.org/wont-fix/

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jdalton

Sure, but that falls under the official Won't Fix :D

Yap, lots of things are out of scope, but that makes the "guarantee" more lip service than substance. So in this case I think it's more beneficial to store the data on the element instead.

With that said, I don't have time to investigate at the moment, so if resources are limited the issue can be noted for future reference.

Member

mgol commented Oct 20, 2014

Comment author: jdalton

Sure, but that falls under the official Won't Fix :D

Yap, lots of things are out of scope, but that makes the "guarantee" more lip service than substance. So in this case I think it's more beneficial to store the data on the element instead.

With that said, I don't have time to investigate at the moment, so if resources are limited the issue can be noted for future reference.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: jzaefferer

We discussed this in Amsterdam. jQuery UI currently depends on cleanData to hook its widget removal in: Whenever an element is removed that is a widget, it calls the destroy() method of the widget, for example to unbind event handlers from the document.

Whatever the solution for this looks like needs to provide some alternative hook for jQuery UI. See also #12213

Member

mgol commented Oct 20, 2014

Comment author: jzaefferer

We discussed this in Amsterdam. jQuery UI currently depends on cleanData to hook its widget removal in: Whenever an element is removed that is a widget, it calls the destroy() method of the widget, for example to unbind event handlers from the document.

Whatever the solution for this looks like needs to provide some alternative hook for jQuery UI. See also #12213

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: m_gol

I've read the comments and it seems there's still an issue with teardown hooks not being invoked if an element is removed via raw DOM methods. Can we overcome this difficulty?

As for the safety guarantees, I mostly agree with jdalton; we're unable to provide complete protection anyway so if (!) it's possible for jQuery-created & handled elements to cooperate with native DOM methods I'm +1 on this one.

Member

mgol commented Oct 20, 2014

Comment author: m_gol

I've read the comments and it seems there's still an issue with teardown hooks not being invoked if an element is removed via raw DOM methods. Can we overcome this difficulty?

As for the safety guarantees, I mostly agree with jdalton; we're unable to provide complete protection anyway so if (!) it's possible for jQuery-created & handled elements to cooperate with native DOM methods I'm +1 on this one.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

Comment author: dmethvin

I'm not worried about complete protection, we're mainly trying to avoid walking all the elements in the tree when we don't have to. If someone mingles jQuery with raw DOM methods to manipulate the document they will need to understand the consequences.

Member

mgol commented Oct 20, 2014

Comment author: dmethvin

I'm not worried about complete protection, we're mainly trying to avoid walking all the elements in the tree when we don't have to. If someone mingles jQuery with raw DOM methods to manipulate the document they will need to understand the consequences.

@timmywil timmywil added the Data label Jan 30, 2015

@timmywil timmywil removed this from the 3.0.0 milestone Jan 30, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 30, 2015

Member

Related: #1728

PR: #1428

Member

timmywil commented Jan 30, 2015

Related: #1728

PR: #1428

timmywil added a commit to timmywil/jquery that referenced this issue Mar 4, 2015

@rwaldron rwaldron closed this in d702b76 Mar 4, 2015

markelog added a commit that referenced this issue Nov 10, 2015

rwaldron added a commit that referenced this issue Nov 10, 2015

Data: move element cache to element[expando]
- avoid explicit data.discard() cleanup calls
- explicitly remove the data.events property, only when private data exists
- reduces code footprint

Fixes gh-1734
Close gh-1428

@lock lock bot locked as resolved and limited conversation to collaborators Jun 20, 2018

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