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

isPlainObject optimize #4510

Closed
wants to merge 3 commits into from
Closed

isPlainObject optimize #4510

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7981ccf
isPlainObject optimize
lioeaet Apr 1, 2023
80c44be
fix Object.create(null), add tests
lioeaet Apr 1, 2023
e7c1c7e
isPlainObject early return upd
lioeaet Apr 1, 2023
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
8 changes: 2 additions & 6 deletions src/utils/isPlainObject.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@
export default function isPlainObject(obj: any): boolean {
if (typeof obj !== 'object' || obj === null) return false

let proto = obj
while (Object.getPrototypeOf(proto) !== null) {
proto = Object.getPrototypeOf(proto)
}

return Object.getPrototypeOf(obj) === proto
const proto = Object.getPrototypeOf(obj)
return Boolean(proto && !Object.getPrototypeOf(proto))
Copy link
Member

Choose a reason for hiding this comment

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

This will fail with Object.create(null), which is a perfectly fine plain object.
See #2599 for some edge case discussions.

Copy link
Author

Choose a reason for hiding this comment

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

great comment, thank you!

there is a complex logic with loop, and make things simpler is profit itself

think we should do this if can iterate less without troubles

case with no proto is fixed in new commit, also added tests for it

speed results almost same

const vars = {
  'date': new Array(7777777).fill(new Date()),
  'array': new Array(7777777).fill([1, 2, 3]),
  'plain obj': new Array(7777777).fill({ x: 1, y: 2 }),
  'model': new Array(7777777).fill(new (class Book {})),
  'null': new Array(7777777).fill(null),
  'undefined': new Array(7777777).fill(undefined),
  'string': new Array(7777777).fill(''),
  'true': new Array(7777777).fill(true),
  'false': new Array(7777777).fill(false),
}

function measure(func, arr) {
  const start = performance.now()
  for (let x of arr) func(x)
  return performance.now() - start
}

function x(obj) {
  if (typeof obj !== 'object' || obj === null) return false

  let proto = obj
  while (Object.getPrototypeOf(proto) !== null) {
    proto = Object.getPrototypeOf(proto)
  }

  return Object.getPrototypeOf(obj) === proto
}

function y(obj) {
  if (!obj) return false

  const proto = obj.__proto__
  return !proto || !Object.getPrototypeOf(proto)
}

const result = {}
for (let k in vars) {
  result[k] = {
    x: measure(x, vars[k]),
    y: measure(y, vars[k]),
  }
}
console.log(result)

Copy link
Member

Choose a reason for hiding this comment

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

I'm just running this in Firefox, and your solution is significantly slower in many cases and only noticeably faster in the new Date case.

There is a way to speed up the existing logic by skipping one Object.getPrototype call with

  if (typeof obj !== 'object' || obj === null) return false

  let proto = obj,
      next
  while ((next = Object.getPrototypeOf(proto)) !== null) {
    proto = next
  }

  return Object.getPrototypeOf(obj) === proto

which seems to be a much safer change that is not potentially breaking.

Please let's step back for a while: Why are you optimizing this? Does this make anything any slower in any real life application you have?

Any change we introduce here might break edge cases we don't know about. Without having a really good reason to, I don't think that code like this should ever be touched.

And a microoptimization is rarely a good reason - so I'm asking again: do you see real impact on this in a real-life application? What is a use case where this makes a difference?

Copy link
Member

Choose a reason for hiding this comment

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

Please stop adding more optimizations.

We will not accept this pull request unless you give us an incredibly good reason to introduce potentially-breaking code to millions of installations.

I have asked multiple times why you are doing this PR in the first place.

Please start talking.

Copy link
Author

Choose a reason for hiding this comment

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

You right, in firefox i can see slower results too.

Can speed up it with replacing first line of code. Results are only positive.

There is no question of UX improvements. Surplus iterations spend energy in real world. It fraught with some surplus damage to ecology. Redux has 7.5 millions of weekly downloads. It is not catastrophic scope for little loop, but it is and can be not.

You shown an hidden error in my code, thank you. But javascript is not endless. Function is simple. We can fix it instantly in case of error.

With excluding cycle code will pretty easer, people will not spend their time on puzzling it.

Totally this pr:

  • prevents unnecessary damage to nature
  • saves code learners time
  • crop code a little
  • may not to include potential bugs, may include (and be fixed)

}