Don't use object pool for initialOldData, and properly recycle tempObject #5459
Conversation
src/core/component.js
Outdated
| @@ -203,6 +204,7 @@ Component.prototype = { | |||
| // If value is an object, copy it to our pooled newAttrValue object to use to update | |||
| // the attrValue. | |||
| tempObject = this.objectPool.use(); | |||
| utils.objectPool.clearObject(tempObject); | |||
There was a problem hiding this comment.
So I understand. use function should already calls clearObject. Is not this redundant?
There was a problem hiding this comment.
Oops, seems when cleaning up the PR I got clearObject and removeUnusedKeys mixed up in this place. The object pool does indeed "clear" the object, but the keys remains. Looking at the code again in more detail, it is actually safe here to have superfluous keys on tempObject in the subsequent utils.extend and extendProperties calls.
| @@ -324,9 +326,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; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
mrxz
force-pushed
the
fix-initial-old-data
branch
from
4308889 to
a565ab3
Compare
|
Thanks! |
Description:
A temporary object was never released to the objectPool, causing a leak. After fixing this leak, some unit tests regressed. It turns out that the
initialOldDataprovided to a component'supdate(oldData)is also leased from this pool. The pool however makes no guarantees about the state of the object. It might very well not be an empty object. (And due to the various leaks, it likely was an empty object)This PR correctly clears and returns the
tempObjectto the pool and uses a constant frozen empty object for theinitialOldData. This is a slight change in behaviour in case users mutate this object, but that was never correct and will now give an error.Changes proposed:
tempObjectin Component update logic (updateCachedAttrValue)initialOldDatafor a Component's firstupdatecall