Skip to content

Conversation

@k-g-a
Copy link
Member

@k-g-a k-g-a commented Aug 7, 2018

Fixes #926

@k-g-a
Copy link
Member Author

k-g-a commented Aug 9, 2018

This one now also supersedes PR #963.
Those two combined give us an opportunity to enrich initialSnapshot with optional values only when it's required (it had been done during construction previously).
I wanted to measue and impact, but as soon as I hit the devtools i saw that it's actually x2 degradation in terms of CPU time and memory.
The reason is that we always get actual snapshot of root, which in turn collects _initialSnapshots of it's children. This used to be a simple object return, but was converted to the loop with childNode.snapshot call for the purpose of this PR. snapshot is computed, so we get much of new arrays, tons of concatinated strings and so on and so forth...
I'll try to improve this part within this week.

- ComplexType.processInitialSnapshot: used to delegate all type-specific code from ObjectNode to concrete type
- Initial performance restored
- removed cloneSnapshotIfNeeded from type.create as it's not needed anymore
- initialSnapshot is frozen
@k-g-a
Copy link
Member Author

k-g-a commented Aug 9, 2018

Commit message outlines the most part, but I'll copy it for convinience:

  • Node.getSnapshot(): uncached version of @computed get snapshot() - by now used to get initial snapshots without creating @computed's overhead
  • ComplexType.processInitialSnapshot: used to delegate all type-specific code from ObjectNode to concrete type - all the this.type instanceof ModelType (and similar) are now removed from ObjectNode
  • Initial performance restored: creation times are similar, little consumed memory raise
  • removed cloneSnapshotIfNeeded (part of PR Do not spoil snapshots during 'create' #963) from type.create as it's not needed anymore
  • initialSnapshot is frozen: to prevent furhter accidental mutations as it happened with optional props

By now I'd prefer this PR over #963 as it combines both + cleanup.
I'd also perfer this one over #962 , as those will conflict, but this one solves issues.

@mweststrate
Copy link
Member

Thanks @k-g-a! Will merge this one to fix the current problems. I think however that we should still keep #947 open to solve this more elegantly in the next major, as the whole snapshot code paths are becoming insanely complex now :)

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.

4 participants