Permalink
Browse files

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
  • Loading branch information...
rwaldron authored and timmywil committed Nov 11, 2013
1 parent 95fb798 commit d702b7637a61e1973e08c27b8d8de2ed24a543e2
Showing with 66 additions and 74 deletions.
  1. +48 −46 src/data/Data.js
  2. +5 −1 src/event.js
  3. +11 −20 src/manipulation.js
  4. +2 −7 test/unit/manipulation.js
View
@@ -5,61 +5,67 @@ define([
], function( jQuery, rnotwhite ) {
function Data() {
// Support: Android<4,
// Old WebKit does not have Object.preventExtensions/freeze method,
// return new empty object instead with no [[set]] accessor
Object.defineProperty( this.cache = {}, 0, {
get: function() {
return {};
}
});
this.expando = jQuery.expando + Data.uid++;
}
Data.uid = 1;
Data.accepts = jQuery.acceptData;
Data.prototype = {
key: function( owner ) {
// We can accept data for non-element nodes in modern browsers,
// but we should not, see #8335.
// Always return the key for a frozen object.
if ( !Data.accepts( owner ) ) {
return 0;
}
// Check if the owner object already has a cache key
var unlock = owner[ this.expando ];
// If not, create one
if ( !unlock ) {
unlock = Data.uid++;
register: function( owner, initial ) {
var descriptor = {},
value = initial || {};
try {
// If it is a node unlikely to be stringify-ed or looped over
// use plain assignment
if ( owner.nodeType ) {
owner[ this.expando ] = unlock;
owner[ this.expando ] = value;
// Otherwise secure it in a non-enumerable, non-writable property
// configurability must be true to allow the property to be
// deleted with the delete operator
} else {
Object.defineProperty( owner, this.expando, { value: unlock } );
descriptor[ this.expando ] = {
value: value,
writable: true,
configurable: true
};
Object.defineProperties( owner, descriptor );

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 5, 2015

Member

Didn't we just get rid of Object.defineProperties? 95fb798

@gibson042

gibson042 Mar 5, 2015

Member

Didn't we just get rid of Object.defineProperties? 95fb798

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 5, 2015

Member

What is the real question you're asking? Would you prefer that this was Object.defineProperty?

@rwaldron

rwaldron Mar 5, 2015

Member

What is the real question you're asking? Would you prefer that this was Object.defineProperty?

}
// Support: Android < 4
// Fallback to a less secure definition
} catch ( e ) {
descriptor[ this.expando ] = value;
jQuery.extend( owner, descriptor );
}
// Ensure the cache object
if ( !this.cache[ unlock ] ) {
this.cache[ unlock ] = {};
return owner[ this.expando ];
},
cache: function( owner, initial ) {
// We can accept data for non-element nodes in modern browsers,
// but we should not, see #8335.
// Always return an empty object.
if ( !Data.accepts( owner ) ) {
return {};
}
return unlock;
// Check if the owner object already has a cache
var cache = owner[ this.expando ];
// If so, return it
if ( cache ) {
return cache;
}
// If not, register one
return this.register( owner, initial );
},
set: function( owner, data, value ) {
var prop,
// There may be an unlock assigned to this node,
// if there is no entry for this "owner", create one inline
// and set the unlock as though an owner entry had always existed
unlock = this.key( owner ),
cache = this.cache[ unlock ];
cache = this.cache( owner );
// Handle: [ owner, key, value ] args
if ( typeof data === "string" ) {
@@ -69,7 +75,8 @@ Data.prototype = {
} else {
// Fresh assignments by object are shallow copied
if ( jQuery.isEmptyObject( cache ) ) {
jQuery.extend( this.cache[ unlock ], data );
jQuery.extend( cache, data );
// Otherwise, copy the properties one-by-one to the cache object
} else {

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 5, 2015

Member

@rwaldron: what is the value of separate execution paths here?

@gibson042

gibson042 Mar 5, 2015

Member

@rwaldron: what is the value of separate execution paths here?

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 5, 2015

Member

Value? Probably none. I'm sure there was probably very good reason at some point—whether that reason holds up today or not is worth checking. I'll look at it tomorrow.

@rwaldron

rwaldron Mar 5, 2015

Member

Value? Probably none. I'm sure there was probably very good reason at some point—whether that reason holds up today or not is worth checking. I'll look at it tomorrow.

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 5, 2015

Member

I think this is vestigial and nothing breaks when it's removed.

@rwaldron

rwaldron Mar 5, 2015

Member

I think this is vestigial and nothing breaks when it's removed.

for ( prop in data ) {
@@ -80,11 +87,7 @@ Data.prototype = {
return cache;
},
get: function( owner, key ) {
// Either a valid cache is found, or will be created.
// New caches will be created and the unlock returned,
// allowing direct access to the newly created
// empty data object. A valid owner object must be provided.
var cache = this.cache[ this.key( owner ) ];
var cache = this.cache( owner );
return key === undefined ?
cache : cache[ key ];
@@ -125,11 +128,10 @@ Data.prototype = {
},
remove: function( owner, key ) {
var i, name, camel,
unlock = this.key( owner ),
cache = this.cache[ unlock ];
cache = this.cache( owner );
if ( key === undefined ) {
this.cache[ unlock ] = {};
this.register( owner );

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 5, 2015

Member

I think this is completely unnecessary, given that it's already covered by the preceding this.cache( owner ).

@gibson042

gibson042 Mar 5, 2015

Member

I think this is completely unnecessary, given that it's already covered by the preceding this.cache( owner ).

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 5, 2015

Member

this is completely unnecessary

When this.register(elem) is called it creates an empty data object, which may be a new data object, or replacing an existing data object. If the owner has data, then this.cache wouldn't even make it to return this.register( owner, initial );—which doesn't do what Data needs it to do at that point anyway.

@rwaldron

rwaldron Mar 5, 2015

Member

this is completely unnecessary

When this.register(elem) is called it creates an empty data object, which may be a new data object, or replacing an existing data object. If the owner has data, then this.cache wouldn't even make it to return this.register( owner, initial );—which doesn't do what Data needs it to do at that point anyway.

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 5, 2015

Member

Sorry, that was my confusion. But since we're here, I'll ask a related question: could we use cache = owner[ this.expando ] instead of cache = this.cache( owner ) to avoid creating data objects when we're trying to remove data?

@gibson042

gibson042 Mar 5, 2015

Member

Sorry, that was my confusion. But since we're here, I'll ask a related question: could we use cache = owner[ this.expando ] instead of cache = this.cache( owner ) to avoid creating data objects when we're trying to remove data?

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 5, 2015

Member

I'll try it out in the am

@rwaldron

rwaldron Mar 5, 2015

Member

I'll try it out in the am

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 5, 2015

Member

could we use cache = owner[ this.expando ] instead of cache = this.cache( owner ) to avoid creating data objects when we're trying to remove data?

Yes. I updated the code and push changes to a new PR

@rwaldron

rwaldron Mar 5, 2015

Member

could we use cache = owner[ this.expando ] instead of cache = this.cache( owner ) to avoid creating data objects when we're trying to remove data?

Yes. I updated the code and push changes to a new PR

} else {
// Support array or space separated string of keys
@@ -156,19 +158,19 @@ Data.prototype = {
}
i = name.length;
while ( i-- ) {
delete cache[ name[ i ] ];
}
}
},
hasData: function( owner ) {
return !jQuery.isEmptyObject(
this.cache[ owner[ this.expando ] ] || {}
);
var cache = owner[ this.expando ];
return cache !== undefined && !jQuery.isEmptyObject( cache );
},
discard: function( owner ) {
if ( owner[ this.expando ] ) {
delete this.cache[ owner[ this.expando ] ];
delete owner[ this.expando ];
}
}
};
View
@@ -216,8 +216,12 @@ jQuery.event = {
// Remove the expando if it's no longer used
if ( jQuery.isEmptyObject( events ) ) {
// Normally this should go through the data api
// but since event.js owns these properties,
// this exception is made for the sake of optimizing
// the operation.
delete elemData.handle;
dataPriv.remove( elem, "events" );
delete elemData.events;
}
},
View
@@ -288,34 +288,25 @@ jQuery.extend({
},
cleanData: function( elems ) {
var data, elem, type, key,
var data, elem, type,
special = jQuery.event.special,
i = 0;
for ( ; (elem = elems[ i ]) !== undefined; i++ ) {
if ( jQuery.acceptData( elem ) ) {
key = elem[ dataPriv.expando ];
if ( key && (data = dataPriv.cache[ key ]) ) {
if ( data.events ) {
for ( type in data.events ) {
if ( special[ type ] ) {
jQuery.event.remove( elem, type );
// This is a shortcut to avoid jQuery.event.remove's overhead
} else {
jQuery.removeEvent( elem, type, data.handle );
}
if ( jQuery.acceptData( elem ) && (data = elem[ dataPriv.expando ])) {
if ( data.events ) {
for ( type in data.events ) {
if ( special[ type ] ) {
jQuery.event.remove( elem, type );
// This is a shortcut to avoid jQuery.event.remove's overhead
} else {
jQuery.removeEvent( elem, type, data.handle );
}
}
if ( dataPriv.cache[ key ] ) {
// Discard any remaining `private` data
delete dataPriv.cache[ key ];
}
}
delete data.events;
}
// Discard any remaining `user` data
delete dataUser.cache[ elem[ dataUser.expando ] ];
}
}
});
@@ -64,7 +64,7 @@ test( "text(undefined)", function() {
function testText( valueObj ) {
expect( 7 );
expect( 6 );
var val, j, expected, $multipleElements, $parentDiv, $childDiv;
@@ -94,8 +94,6 @@ function testText( valueObj ) {
$parentDiv = jQuery( "<div/>" );
$parentDiv.append( $childDiv );
$parentDiv.text("Dry off");
equal( $childDiv.data("leak"), undefined, "Check for leaks (#11809)" );
}
test( "text(String)", function() {
@@ -1814,7 +1812,7 @@ test( "clone()/html() don't expose jQuery/Sizzle expandos (#12858)", function()
test( "remove() no filters", function() {
expect( 3 );
expect( 2 );
var first = jQuery("#ap").children().first();
@@ -1823,9 +1821,6 @@ test( "remove() no filters", function() {
jQuery("#ap").children().remove();
ok( jQuery("#ap").text().length > 10, "Check text is not removed" );
equal( jQuery("#ap").children().length, 0, "Check remove" );
equal( first.data("foo"), null, "first data" );
});
test( "remove() with filters", function() {

2 comments on commit d702b76

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Mar 8, 2015

Member

This commit caused failures with olddisplay in css module - http://swarm.jquery.org/result/25273

/cc @timmywil

Member

markelog replied Mar 8, 2015

This commit caused failures with olddisplay in css module - http://swarm.jquery.org/result/25273

/cc @timmywil

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 8, 2015

Member

Whoa, consistently... The error I saw locally was a CSP error, which I just ignored. I'll take a look at this.

Member

rwaldron replied Mar 8, 2015

Whoa, consistently... The error I saw locally was a CSP error, which I just ignored. I'll take a look at this.

Please sign in to comment.