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

Avoid unnecesary prototype loop #581

Closed
wants to merge 1 commit into from

Conversation

WebReflection
Copy link

As the whole purpose of the while (Object.getPrototypeOf(proto) !== null) loop seems to counter validate there was nothing else to loop, I believe it's a performance benefit, as well as smaller code, to simply check the prototype never more than 2 levels, with a graceful fallback in case the first check resulted into null already:

  • the amount of Object.getPrototypeOf is fixed as 2 and never more
  • there is less code to shrink/optimize for bundlers or minifiers

As the whole purpose of the `while (Object.getPrototypeOf(proto) !== null)` loop seems to counter validate there was nothing else to loop, I believe it's a performance benefit, as well as smaller code, to simply check the prototype never more than 2 levels, with a graceful fallback in case the first check resulted into `null` already:
  * the amount of `Object.getPrototypeOf` is fixed as `2` and never more
  * there is less code to shrink/optimize for bundlers or minifiers
@netlify
Copy link

netlify bot commented May 25, 2020

Deploy preview for redux-starter-kit-docs ready!

Built with commit e27b788

https://deploy-preview-581--redux-starter-kit-docs.netlify.app

@codesandbox-ci
Copy link

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 e27b788:

Sandbox Source
cold-wind-yyez2 Configuration
vigorous-fire-h1iu9 Configuration
lingering-water-pv986 Configuration

return (
typeof value === 'object' &&
value !== null &&
Object.getPrototypeOf(Object.getPrototypeOf(value) || 0) === null
Copy link
Author

@WebReflection WebReflection May 25, 2020

Choose a reason for hiding this comment

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

P.S. the 0 could be just a literal {} instead, it'd work exactly the same (but it'd be 1 extra char).

@markerikson
Copy link
Collaborator

This was originally implemented by @timdorr and we copied it from the Redux core.

I'm not inclined to change it unless there's truly a specific need, but I'll let him weigh in on it.

@timdorr
Copy link
Member

timdorr commented May 25, 2020

Yeah, I'm not sure I really see the benefit here. Thanks, but this is fine as-is.

@timdorr timdorr closed this May 25, 2020
@WebReflection
Copy link
Author

I'm not sure I really see the benefit here

is performance a concern of this project?

this brings O(1) vs O(n) on the plate, so it's faster, but also smaller.

every class based object, or Object.create(other) one, will crawl more than twice the prototype for no reason, and copy-pasting the initial solution shouldn't block this from improving, right?

WebReflection added a commit to WebReflection/redux that referenced this pull request May 25, 2020
## Background

People at redux-toolkit copy/pasted this utility in there, and [they are not welcoming a PR](reduxjs/redux-toolkit#581) because of that, but the `while` loop in here is completely unnecessary, as the comparison is made on the first `proto` only.

## Improvements

  * _O(1)_ instead of _O(n)_
  * safe fallback for `Object.create(null)` cases
  * fast lane for `new Class()` derived objects
  * better performance
  * smaller code size

As this change has zero downsides, unless I am missing some explicit reason to `while` loop instead of never using `getPrototypeOf` more than twice, I hope it'll get in, so that others might copy and paste this new version.

Best Regards.
@timdorr
Copy link
Member

timdorr commented May 25, 2020

Theoretically, this runs in linear time, but practically it's constant time. The vast majority of users are passing in plain objects where this check is run. Those will have a single level prototype chain, meaning this while loop will run once and essentially becomes an if statement.

Regardless, we've been over this a number of times. The previous conclusion is this code isn't in the hot path of execution, so perfecting the execution time isn't critical and doesn't net any visible benefit. Even if it was, the compiler is going to optimize it anyway and it won't end up being more than a few opcodes.

@WebReflection
Copy link
Author

WebReflection commented May 25, 2020

The vast majority of users are passing in plain objects where this check is run. Those will have a single level prototype chain, meaning this while loop will run once and essentially becomes an if statement.

current code runs Object.getPrototypeOf at least twice or more, my code runs it never more than twice.

this code isn't in the hot path of execution, so perfecting the execution time isn't critical and doesn't net any visible benefit

to me it's a matter of logic. The while loop is redundant, and the if could be considered an edge case, covered by || 0 in this proposed change.

the compiler is going to optimize it anyway and it won't end up being more than a few opcodes.

if it's not critical path I understand, but the current code executes more instructions than it needs.

Metaphorically speaking, why would you prefer this code:

let current = i;
while (i--);
return (i + 1) === current;

instead of this?

return --i === 0;

anyway, your projects, your rules, I just personally welcome simple improvements, when no regressions whatsoever are presented, only benefits (even if logical, or theoretical).

Best Regards

WebReflection added a commit to WebReflection/redux that referenced this pull request May 25, 2020
## Background

People at redux-toolkit copy/pasted this utility in there, and [they are not welcoming a PR](reduxjs/redux-toolkit#581) because of that, but the `while` loop in here is completely unnecessary, as the comparison is made on the first `proto` only.

## Improvements

  * _O(1)_ instead of _O(n)_
  * safe fallback for `Object.create(null)` cases
  * fast lane for `new Class()` derived objects
  * better performance
  * smaller code size

As this change has zero downsides, unless I am missing some explicit reason to `while` loop instead of never using `getPrototypeOf` more than twice, I hope it'll get in, so that others might copy and paste this new version.

Best Regards.
WebReflection added a commit to WebReflection/redux that referenced this pull request May 25, 2020
## Background

People at redux-toolkit copy/pasted this utility in there, and [they are not welcoming a PR](reduxjs/redux-toolkit#581) because of that, but the `while` loop in here is completely unnecessary, as the comparison is made on the first `proto` only.

## Improvements

  * _O(1)_ instead of _O(n)_
  * safe fallback for `Object.create(null)` cases
  * fast lane for `new Class()` derived objects
  * better performance
  * smaller code size

As this change has zero downsides, unless I am missing some explicit reason to `while` loop instead of never using `getPrototypeOf` more than twice, I hope it'll get in, so that others might copy and paste this new version.

Best Regards.
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