Optimize mergeProps and splitProps
#1710
Conversation
🦋 Changeset detectedLatest commit: 6f3c5b3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Pull Request Test Coverage Report for Build 4944594073Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
Interesting. I didn't realize that, so getting property descriptor n times in a loop is faster than that getting all them all at once in the plural form. That's good to know. |
juanrgm
force-pushed
the
perf-props
branch
5 times, most recently
from
d903d8d to
a69dd00
Compare
|
With the latest changes the tests are successful (I still need to add more tests for Not generating breaking changes means a loss of performance, even so the improvement is noticeable (x2). In the next merge request you can discuss which improvements to unlock (commented out in the code with reportBefore merging this branch, I'll remove the component.old.tsimport { $PROXY, MergeProps, SplitProps, createMemo } from "../src";
import { EffectFunction } from "../types";
function trueFn() {
return true;
}
function resolveSource(s: any) {
return !(s = typeof s === "function" ? s() : s) ? {} : s;
}
const propTraps: ProxyHandler<{
get: (k: string | number | symbol) => any;
has: (k: string | number | symbol) => boolean;
keys: () => string[];
}> = {
get(_, property, receiver) {
if (property === $PROXY) return receiver;
return _.get(property);
},
has(_, property) {
if (property === $PROXY) return true;
return _.has(property);
},
set: trueFn,
deleteProperty: trueFn,
getOwnPropertyDescriptor(_, property) {
return {
configurable: true,
enumerable: true,
get() {
return _.get(property);
},
set: trueFn,
deleteProperty: trueFn
};
},
ownKeys(_) {
return _.keys();
}
};
export function splitProps<
T extends Record<any, any>,
K extends [readonly (keyof T)[], ...(readonly (keyof T)[])[]]
>(props: T, ...keys: K): SplitProps<T, K> {
const blocked = new Set<keyof T>(keys.length > 1 ? keys.flat() : keys[0]);
if ($PROXY in props) {
const res = keys.map(k => {
return new Proxy(
{
get(property) {
return k.includes(property) ? props[property as any] : undefined;
},
has(property) {
return k.includes(property) && property in props;
},
keys() {
return k.filter(property => property in props);
}
},
propTraps
);
});
res.push(
new Proxy(
{
get(property) {
return blocked.has(property) ? undefined : props[property as any];
},
has(property) {
return blocked.has(property) ? false : property in props;
},
keys() {
return Object.keys(props).filter(k => !blocked.has(k));
}
},
propTraps
)
);
return res as SplitProps<T, K>;
}
const descriptors = Object.getOwnPropertyDescriptors(props);
keys.push(Object.keys(descriptors).filter(k => !blocked.has(k as keyof T)) as (keyof T)[]);
return keys.map(k => {
const clone = {};
for (let i = 0; i < k.length; i++) {
const key = k[i];
if (!(key in props)) continue; // skip defining keys that don't exist
Object.defineProperty(
clone,
key,
descriptors[key]
? descriptors[key]
: {
get() {
return props[key];
},
set() {
return true;
},
enumerable: true
}
);
}
return clone;
}) as SplitProps<T, K>;
}
export function mergeProps<T extends unknown[]>(...sources: T): MergeProps<T> {
let proxy = false;
for (let i = 0; i < sources.length; i++) {
const s = sources[i];
proxy = proxy || (!!s && $PROXY in (s as object));
sources[i] =
typeof s === "function" ? ((proxy = true), createMemo(s as EffectFunction<unknown>)) : s;
}
if (proxy) {
return new Proxy(
{
get(property: string | number | symbol) {
for (let i = sources.length - 1; i >= 0; i--) {
const v = resolveSource(sources[i])[property];
if (v !== undefined) return v;
}
},
has(property: string | number | symbol) {
for (let i = sources.length - 1; i >= 0; i--) {
if (property in resolveSource(sources[i])) return true;
}
return false;
},
keys() {
const keys = [];
for (let i = 0; i < sources.length; i++)
keys.push(...Object.keys(resolveSource(sources[i])));
return [...new Set(keys)];
}
},
propTraps
) as unknown as MergeProps<T>;
}
const target = {} as MergeProps<T>;
for (let i = sources.length - 1; i >= 0; i--) {
if (sources[i]) {
const descriptors = Object.getOwnPropertyDescriptors(sources[i]);
for (const key in descriptors) {
if (key in target) continue;
Object.defineProperty(target, key, {
enumerable: true,
get() {
for (let i = sources.length - 1; i >= 0; i--) {
const v = ((sources[i] as any) || {})[key];
if (v !== undefined) return v;
}
}
});
}
}
}
return target;
} |
|
This looks good.. can you expand on the |
|
Uncommenting the
But it will also increase performance. The conservative option it would be to not break the tests in That tests are touching the limits and breaking them should not cause problems in the ecosystem because are undocumented features, but I don't know. What do you think? |
|
Thanks. Yeah some of these might be fine now. A lot of the tests were written a certain way for convenience not because we cared about these things. But you are right it is safer not to introduce the changes to 1.8. Let me review them. |
|
On review I think the only intended behavior here was that a single argument clones. And possibly that prop objects could be overridden. But all the identity stuff for straight values was not intended. So I think that will be safe for 1.7 release. We can review the others later. |
Summary
Object.definePropertiesis slower thanObject.defineProperty.Object.getOwnPropertyDescriptorsis slower thanObject.getOwnPropertyDescriptor.Object.definePropertyis slower than to assign a value.How did you test this change?
cd packages/solid npm treport
report
report
report