Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(copy): do not copy the same object twice #11215

Closed
wants to merge 1 commit into from

Conversation

jbedard
Copy link
Collaborator

@jbedard jbedard commented Mar 2, 2015

#11099 made me curious if there are any easy performance improvements in copy. Instead I found some weird edge cases and inconsistencies. Performance also improved in all the cases I tried (~25% less time & memory in the latest dataset I've been testing, but this will vary greatly depending on data).

Changes:

  • Move the stack check before creating new objects instead of before recursing them. This avoids creating objects/arrays just to throw them out in the recursive call. This fixes non-recursed objects being duplicated when there are multiple references.
  • Move the stack push to after creating new objects instead of after the recursive call to avoid object/arrays being pushed twice. The extra return on the recursive calls is also needed for this.
  • Added various tests that were useful while testing different solutions. The 'should handle date/regex objects with multiple references' is the only one that previously failed.

@jbedard
Copy link
Collaborator Author

jbedard commented Jun 6, 2015

Rebased. Anyone have a chance to look at this? I'd really like to do a few more things here but was hoping to get this in first to avoid an even more complicated commit...

@lgalfaso
Copy link
Contributor

lgalfaso commented Jun 7, 2015

Overall, I like the changes. Just a few comments:

  • I do not expect a 25% performance improvements in the general case, just when there is a high self-referential or the same Date or RegEx objects used multiple times. Is this the case?
  • The reason why in the case without a destination we sometimes return and sometimes do not, is not clear. A comment would be highly welcome.
  • All the checks on stackSource make me think if the main recursive function should be an inner function and these parameters removed. Ie
function copy(source, destination) {
  var stackSource = [];
  var stackDest = [];

  copy_(source, destination);

  function copy_(source, destination) {
    // Recursive logic goes here, but without the checks on stackSource.
  }
}

??

@jbedard jbedard force-pushed the 11099-copy-perf branch from 845d17e to a381ee0 Compare June 7, 2015 17:37
@jbedard
Copy link
Collaborator Author

jbedard commented Jun 7, 2015

I've added a comment above the confusing sometimes-returning block.

Almost all cases seem to be ~25% faster. I think for a few reasons...

  • Previously nested object/arrays were pushed onto the stack twice (=> indexOf is more expensive?) A + B.
  • Previously isObject(result) and the removed function call were done per recursive call. Now that the stack.push has moved this isn't necessary.
  • Previously isArray/isRegExp/isTypedArray/isObject were called on every copied value. Now these are only called if (isObject(source)). This probably makes the biggest difference.

@lgalfaso
Copy link
Contributor

lgalfaso commented Jun 8, 2015

landed as 0e622f7

@lgalfaso lgalfaso closed this Jun 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants