Skip to content

Data: move element cache to element[expando] #1428

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Data: move element cache to element[expando] #1428

wants to merge 1 commit into from

Conversation

rwaldron
Copy link
Member

  • avoid explicit data.discard() cleanup calls
  • explicitly remove the data.events property
  • reduces code footprint

Fixes #1734

@dmethvin
Copy link
Member

Ya know, this just might work.

I'm thinking with this in place we may be able (in a separate commit) to optimize cleanData() to only clean up elements if they have events with special event handlers. I haven't puzzled it all the way through but we may need to remove all handlers to prevent leaks from closures. But I'm hoping that we can avoid the data cache cleanup entirely for most cases. jQuery UI duck punches ahead of that call so I don't think it should break there.

@dmethvin dmethvin added this to the 1.12/2.2 milestone Feb 7, 2014
@rwaldron
Copy link
Member Author

rwaldron commented May 7, 2014

Rebased (waiting on travis at the moment)

@rwaldron
Copy link
Member Author

rwaldron commented May 7, 2014

This is failing many tests in manipulation now

@rwaldron
Copy link
Member Author

rwaldron commented May 7, 2014

Fixed issues with manipulation, these were caused by changes that didn't carry over after rebasing.

dcherman
dcherman reviewed Aug 11, 2014
View reviewed changes
src/data/Data.js
@@ -167,12 +157,12 @@ Data.prototype = {
},
hasData: function( owner ) {
return !jQuery.isEmptyObject(
this.cache[ owner[ this.expando ] ] || {}
owner[ this.expando ] || {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than doing this, we should do something like

return this.expando in owner && !jQuery.isEmptyObject( owner[this.expando] );

Which avoids needlessly creating an empty object if the cache didn't exist on the element

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is written to match the semantics of 1.x data; the object has no bindings and therefore no references to keep it in memory. Is there a real memory threat that exceeds the cost of using an in operation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In jquery/jquery-ui#1309 I'm trying to get them to use $.hasData in the pseudo selectors that they've created in order to avoid potential memory leaks on sets containing detached nodes. Assuming that's accepted, this would avoid creating N objects where N is the # of DOM nodes that do not not currently have data in the case that you do something like $( this ).find( ":ui-spinner" );

If there's concern about the cost of the in operator, you could also do

var cache = owner[ this.expando ]; 
return !!cache && !jQuery.isEmptyObject( cache );

which has no additional cost since both the old and new implementations both have a single property access in them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the reality check.

@rwaldron rwaldron mentioned this pull request Sep 28, 2014
Closed
@mgol mgol mentioned this pull request Oct 20, 2014
Closed
@timmywil timmywil added the Data label Dec 10, 2014
@rwaldron rwaldron force-pushed the data-as-expando branch 3 times, most recently from 7989bab to 9e7fd6c Compare December 18, 2014 16:32
@timmywil timmywil mentioned this pull request Jan 30, 2015
Closed
timmywil pushed a commit to timmywil/jquery that referenced this pull request Mar 4, 2015
@jbedard @timmywil
Data: avoid Object.defineProperties for nodes
95fb798 
Closes jquerygh-1668
Fixes jquerygh-1728
Ref jquerygh-1734
Ref jquerygh-1428
@timmywil
Copy link
Member

timmywil commented Mar 4, 2015

@rwaldron Could you resolve the conflicts. I think this is ready after that.

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

Fixes #11570

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
@rwaldron
Copy link
Member Author

rwaldron commented Mar 4, 2015

  • Rebased
  • Resigned the CLA
  • Updated to include changes based on @dcherman's feedback

@rwaldron
Copy link
Member Author

rwaldron commented Mar 4, 2015

weird

@rwaldron rwaldron closed this in d702b76 Mar 4, 2015
@jbedard
Copy link
Contributor

jbedard commented Mar 4, 2015

Some of the cleanup from 95fb798 got undone here.

defineProperties was replaced with defineProperty, then descriptor can be removed (less code/vars and it is minorly faster). The Android try/catch was also removed. Both of these have been added back in d702b76

@timmywil
Copy link
Member

timmywil commented Mar 4, 2015

You're right. @rwaldron was this intentional or is it alright to change back?

markelog pushed a commit that referenced this pull request Nov 10, 2015
@jbedard @markelog
Data: avoid Object.defineProperties for nodes
4b6f562 
Closes gh-1668
Fixes gh-1728
Ref gh-1734
Ref gh-1428
rwaldron added a commit that referenced this pull request Nov 10, 2015
@rwaldron @markelog
Data: move element cache to element[expando]
7b2f017 
- 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 Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Data
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

Move element cache to the element[expando] to avoid cleanup and reduce code.
6 participants
@rwaldron @dmethvin @timmywil @jbedard @dcherman @jquerybot