Skip to content

Don't use object pool for initialOldData, and properly recycle tempObject #5459

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

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Don't use object pool for initialOldData, and properly recycle tempObject #5459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/a-entity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ class AEntity extends ANode {
/**
* Initialize component.
*
* @param {string} attrName - Attribute name asociated to the component.
* @param {string} attrName - Attribute name associated to the component.
* @param {object} data - Component data
* @param {boolean} isDependency - True if the component is a dependency.
*/
Expand Down
8 changes: 4 additions & 4 deletions src/core/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var upperCaseRegExp = new RegExp('[A-Z]+');

// Object pools by component, created upon registration.
var objectPools = {};
var emptyInitialOldData = Object.freeze({});

/**
* Component class definition.
Expand Down Expand Up @@ -223,7 +224,7 @@ Component.prototype = {
}
utils.objectPool.clearObject(this.attrValue);
this.attrValue = extendProperties(this.attrValue, newAttrValue, this.isObjectBased);
utils.objectPool.clearObject(tempObject);
this.objectPool.recycle(tempObject);
},

/**
Expand Down Expand Up @@ -271,7 +272,7 @@ Component.prototype = {
*
* @param {string} attrValue - HTML attribute value.
* If undefined, use the cached attribute value and continue updating properties.
* @param {boolean} clobber - The previous component data is overwritten by the atrrValue.
* @param {boolean} clobber - The previous component data is overwritten by the attrValue.
*/
updateProperties: function (attrValue, clobber) {
var el = this.el;
Expand Down Expand Up @@ -324,9 +325,8 @@ Component.prototype = {

// For oldData, pass empty object to multiple-prop schemas or object single-prop schema.
// Pass undefined to rest of types.
initialOldData = this.isObjectBased ? this.objectPool.use() : undefined;
initialOldData = this.isObjectBased ? emptyInitialOldData : undefined;
Copy link
Member

@dmarcos dmarcos Feb 12, 2024

Choose a reason for hiding this comment

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

Same here. My understanding is that the use function calls clearObject. You mention

The pool however makes no guarantees about the state of the object. It might very well not be an empty object

What Am I missing?

Copy link
Contributor Author

@mrxz mrxz Feb 12, 2024

Choose a reason for hiding this comment

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

The problem is that clearObject only sets the keys to undefined. As a result the initialOldData can have zero or more keys present. This means that any update code doing things like Object.keys(oldData) or "someProp" in oldData ends up getting incorrect results.

Before there was enough leakage that you'd generally always get a fresh empty object from the pool.

this.update(initialOldData);
if (this.isObjectBased) { this.objectPool.recycle(initialOldData); }

// Play the component if the entity is playing.
if (el.isPlaying) { this.play(); }
Expand Down