Deduplicate object creation code in a non-branched setting #2302
Conversation
| @@ -278,6 +278,7 @@ export class ResidualHeapVisitor { | |||
| } | |||
|
|
|||
| visitObjectProperties(obj: ObjectValue, kind?: ObjectKind): void { | |||
| // In normal mode, properties of leaked objects are generated via assignments | |||
|
Thanks! Actually, this change no longer passes the React internal bundle. It did a while ago. Looking into the source of the regression. |
| @@ -1692,13 +1692,26 @@ export class ResidualHeapSerializer { | |||
| let remainingProperties = new Map(val.properties); | |||
| const dummyProperties = new Set(); | |||
| let props = []; | |||
| let isFunction = f => f instanceof ECMAScriptSourceFunctionValue; | |||
There was a problem hiding this comment.
It is neither clear to me that it is correct to just check for instances of ECMAScriptSourceFunctionValue nor that it is a good idea to name this check "isFunction".
|
|
||
| // TODO #2259: Make deduplication in the face of leaking work for custom accessors | ||
| let shouldDropAsAssignedProp = (descriptor: Descriptor) => | ||
| isCertainlyLeaked && !(isFunction(descriptor.get) || isFunction(descriptor.set)); |
There was a problem hiding this comment.
There is already a Spec method: IsDataDescriptor. It seems to me that isCertainlyLeaked && IsDataDescriptor(descriptor) would be safer and more in keeping with existing conventions.
sb98052
force-pushed
the
deduplicate-objects
branch
from
d7a5a69
to
17ccc70
Compare
|
|
||
| // TODO #2259: Make deduplication in the face of leaking work for custom accessors | ||
| let shouldDropAsAssignedProp = (descriptor: Descriptor) => | ||
| isCertainlyLeaked && !IsDataDescriptor(descriptor); |
There was a problem hiding this comment.
The original logic for this seemed more consistent to "isCertainlyLeaked && IsDataDescriptor(descriptor)".
sb98052
force-pushed
the
deduplicate-objects
branch
from
17ccc70
to
18beaa4
Compare
|
Kindly ignore changes on this PR. I am coordinating with @trueadm at the moment, who is helping debug the failure with the internal bundle. |
sb98052
force-pushed
the
deduplicate-objects
branch
2 times, most recently
from
b2cf63a
to
ca2b84d
Compare
| @@ -324,11 +324,20 @@ export class ResidualHeapSerializer { | |||
| ); | |||
| } | |||
|
|
|||
| // TODO #2259: Make deduplication in the face of leaking work for custom accessors | |||
| let isCertainlyLeaked = !obj.mightNotBeHavocedObject(); | |||
There was a problem hiding this comment.
Do you see a away to consolidate these two instances of iterating over properties?
If not, it's fine as it is.
There was a problem hiding this comment.
I will look into it, and submit a separate pull request if I come up with something.
sb98052
force-pushed
the
deduplicate-objects
branch
from
ca2b84d
to
83aeb72
Compare
There was a problem hiding this comment.
@sb98052 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request breaks out the first part of #2185, which deduplicates object creation when leaking is unconditional to begin with. No changes are made to how leaking works. Only to how leak information is used by the serializer and visitor.