Skip to content
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

Jump to bottom

improve performance of updateElement #321

Merged
merged 1 commit into from
Nov 8, 2021
Merged

improve performance of updateElement #321

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
36 changes: 27 additions & 9 deletions src/dom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,40 @@
import { Attributes, DOM, IFiber } from "./type"
import { isStr, LANE } from "./reconcile"

const hasOwnProperty = Object.prototype.hasOwnProperty;

const defaultObj = {} as const;

const jointIter = <P extends Attributes>(
aProps = defaultObj as P,
bProps = defaultObj as P,
callback: (name: string, a: any, b: any) => void
) => {
for (const name in aProps) {
if (hasOwnProperty.call(aProps, name)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be name in aProps? Maybe can save bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modern JS engines do some optimizations for the for ... in loops, followed by hasOwnProperty check, so this might be needed for getting maximum performance

Copy link
Collaborator

@yisar yisar Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I tried Object.keys. It will be faster and have fewer bytes.

const jointIter = <P extends Attributes>(
  aProps = defaultObj as P,
  bProps = defaultObj as P,
  callback: (name: string, a: any, b: any) => void
) => {
  Object.keys(aProps).forEach(k => callback(k, aProps[k], bProps[k]))
  Object.keys(bProps).forEach(k => callback(k, null, bProps[k]))
}

Do you agree with me to make some changes on your code?

Copy link
Contributor Author

@zheksoon zheksoon Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hah, I've also discovered that Object.keys and forEach performs almost the same as for ... in loop. But your version has an issue: it will iterate twice by properties that are both in aProps and bProps. Correct version is this one:

const jointIter = <P extends Attributes>(
  aProps = defaultObj as P,
  bProps = defaultObj as P,
  callback: (name: string, a: any, b: any) => void
) => {
  Object.keys(aProps).forEach((k) => {
    callback(k, aProps[k], bProps[k]);
  });
  Object.keys(bProps).forEach((k) => {
    if (!hasOwnProperty.call(aProps, k)) {
      callback(k, undefined, bProps[k]);
    }
  });
}

I'd also rather prefer returning undefined to the callback as this is consistent with your previous implementation

Copy link
Collaborator

@yisar yisar Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You're right. Thank you. I'll revise it again.

I'd also rather prefer returning undefined to the callback as this is consistent with your previous implementation.

What does this mean? I like your implementation better than mine.

Copy link
Contributor Author

@zheksoon zheksoon Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What does this mean? I like your implementation better than mine.

It's just for compatibility with your previous implementation.

https://github.com/yisar/fre/blob/afea19f5b3d7ca91025f2f1c2d812f27c70397ee/src/dom.ts#L11
There is a problem with this check - it won't iterate any values at all if it will be called as jointIter(null, { a: 1 }, callback) or jointIter({ a: 1 }, null, callback) - this case usually can't happen with usual props, but can happen with style prop.

And just realized that my implementation has the flaw with null argument too - it will throw with any null argument. The right one will be like this:

const jointIter = <P extends Attributes>(
  aProps: P,
  bProps: P,
  callback: (name: string, a: any, b: any) => void
) => {
  aProps = aProps || defaultObj;
  bProps = bProps || defaultObj;
  Object.keys(aProps).forEach((k) => {
    callback(k, aProps[k], bProps[k]);
  });
  Object.keys(bProps).forEach((k) => {
    if (!hasOwnProperty.call(aProps, k)) {
      callback(k, undefined, bProps[k]);
    }
  });
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

done! Thank u!

callback(name, aProps[name], bProps[name]);
}
}
for (const name in bProps) {
if (hasOwnProperty.call(bProps, name) && !hasOwnProperty.call(aProps, name)) {
callback(name, undefined, bProps[name]);
}
}
}

export const updateElement = <P extends Attributes>(
dom: DOM,
aProps: P,
bProps: P
) => {
for (let name in { ...aProps, ...bProps }) {
let a = aProps[name]
let b = bProps[name]

jointIter(aProps, bProps, (name, a, b) => {
if (a === b || name === "children") {
} else if (name === "style" && !isStr(b)) {
for (const k in { ...a, ...b }) {
if (!(a && b && a[k] === b[k])) {
; (dom as any)[name][k] = b?.[k] || ""
jointIter(a, b, (styleKey, aStyle, bStyle) => {
if (aStyle !== bStyle) {
; (dom as any)[name][styleKey] = bStyle || ""
}
}
})
} else if (name[0] === "o" && name[1] === "n") {
name = name.slice(2).toLowerCase() as Extract<keyof P, string>
if (a) dom.removeEventListener(name, a)
Expand All @@ -28,7 +46,7 @@ export const updateElement = <P extends Attributes>(
} else {
dom.setAttribute(name, b)
}
}
})
}

export const createElement = <P = Attributes>(fiber: IFiber) => {
Expand Down