Skip to content

Conversation

@B4nan
Copy link
Member

@B4nan B4nan commented Oct 9, 2022

Previously Collection.add/remove were defined with a spread parameter, which could result in perf degradation in very large collections. This change makes the first parameter accept also an array of items, skipping unnecessary copying of arrays when hydarting the collection.

This is slightly breaking on type level, as it is no longer possible to do coll.add(...items) with a generic array - it only works with a tuple. The solution is to do coll.add(items) instead, which will type check and is also more performant (no array copies).

Closes #3572

Previously `Collection.add/remove` were defined with a spread parameter, which could result in perf
degradation in very large collections. This change makes the first parameter accept also an array of
items, skipping unnecessary copying of arrays when hydarting the collection.

This is slightly breaking on type level, as it is no longer possible to do `coll.add(...items)` with
a generic array - it only works with a tuple. The solution is to do `coll.add(items)` instead, which
will type check and is also more performant (no array copies).

Closes #3572
@codecov-commenter
Copy link

Codecov Report

Base: 99.87% // Head: 99.84% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (fc33dc5) compared to base (8424976).
Patch coverage: 93.61% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3573      +/-   ##
==========================================
- Coverage   99.87%   99.84%   -0.04%     
==========================================
  Files         211      211              
  Lines       13131    13147      +16     
  Branches     3049     3051       +2     
==========================================
+ Hits        13115    13127      +12     
- Misses         16       20       +4     
Impacted Files Coverage Δ
packages/core/src/entity/ArrayCollection.ts 97.18% <85.71%> (-2.82%) ⬇️
packages/core/src/entity/Collection.ts 100.00% <100.00%> (ø)
packages/core/src/entity/EntityLoader.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@B4nan B4nan force-pushed the perf-large-collections branch from fc33dc5 to 6a792bc Compare October 9, 2022 15:59
@B4nan B4nan merged commit ea3f6fd into master Oct 9, 2022
@B4nan B4nan deleted the perf-large-collections branch October 9, 2022 16:03
@B4nan B4nan restored the perf-large-collections branch January 10, 2023 19:12
@B4nan B4nan deleted the perf-large-collections branch January 10, 2023 19:18
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.

crash because of "unfortunate" spread operator in collection.modify (fix is to replace the spread by array passing)

3 participants