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.

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

wants to merge 3 commits into from

Conversation

lioeaet
Copy link

@lioeaet lioeaet commented Apr 1, 2023

hi

optimization of isPlainObject function is in this pr

sorry for such few words, language barrier it is

performance tests:

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 (typeof obj !== 'object' || obj === null) return false

  const proto = Object.getPrototypeOf(obj)
  return Boolean(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)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 1, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e7c1c7e:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

Comment on lines 8 to 9
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)

@phryneas
Copy link
Member

phryneas commented Apr 1, 2023

Generally: how many actions are you dispatching that this results in any relevant performance improvements? This check should already work millions per second without problems.

@markerikson
Copy link
Contributor

Hi. We appreciate the PR, but the current implementation is okay as-is. This function is not going to be a perf bottleneck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants