Significantly reduced performance when adding a large object and modifying a Set with proxies enabled and autofreeze disabled

[srubin]

🐛 Bug Report

When proxies are enabled and autofreeze is disabled (i.e., the default production settings for immer), immer demonstrates significantly reduced performance when adding a large object and modifying a Set in the same producer.

This type of call will demonstrate the reduced performance:

const dataSet = require('./data.json') // some large object
const baseState = {
    data: null,
    data2: new Set()
}
produce(baseState, draft => {
    draft.data = dataSet
    draft.data2.add(1)
})

Immer adds an additional internal draft when mutating a set. That causes this optimization to be skipped because rootScope.unfinalizedDrafts_ < 1 is false. In turn, finalizeProperty is called recursively for every property of the large data object.

When I profiled this test case, 50% of the runtime was in calls to isDraftable and isDraft in finalizeProperty.

Link to repro

The performance test case added in this PR demonstrates the issue: descriptinc#1

To Reproduce

1. Checkout the branch sr/performance-with-multiple-drafts in the linked fork.
2. Run yarn run babel-node __performance_tests__/multiple-drafts.js

Observed behavior

The output is along the lines of:

# multiple-drafts - loading large set of data and updating a Set

immer (proxy) - without autofreeze * 1000: 6977ms
immer (proxy) - with autofreeze * 1000: 32ms
immer (es5) - without autofreeze * 1000: 35ms
immer (es5) - with autofreeze * 1000: 43ms

The first case has a median runtime of >200x the other cases, which is unexpectedly poor performance.

Expected behavior

I would expect that the runtime of the first performance to be closer to that of the other cases.

Environment

• Immer version: master
• Occurs with setUseProxies(true)
• Occurs with setUseProxies(false) (ES5 only)

[mweststrate]

To optimize this, call Object.freeze first on the dataSet, it will prevent Immer from inspecting the full data set to find drafts

…

On Wed, May 13, 2020 at 3:37 PM Steve Rubin ***@***.***> wrote: 🐛 Bug Report When proxies are enabled and autofreeze is disabled (i.e., the default production settings for immer), immer demonstrates significantly reduced performance when adding a large object and modifying a Set in the same producer. This type of call will demonstrate the reduced performance: const dataSet = require('./data.json') // some large object const baseState = { data: null, data2: new Set() } produce(baseState, draft => { draft.data = dataSet draft.data2.add(1) }) Immer adds an additional internal draft when mutating a set. That causes this optimization <https://github.com/immerjs/immer/blob/master/src/core/finalize.ts#L142> to be skipped because rootScope.unfinalizedDrafts_ < 1 is false. In turn, finalizeProperty is called recursively for every property of the large data object. When I profiled this test case, 50% of the runtime was in calls to isDraftable and isDraft in finalizeProperty. Link to repro The performance test case added in this PR demonstrates the issue: descriptinc#1 <descriptinc#1> To Reproduce 1. Checkout the branch sr/performance-with-multiple-drafts in the linked fork. <descriptinc#1> 2. Run yarn run babel-node __performance_tests__/multiple-drafts.js Observed behavior The output is along the lines of: # multiple-drafts - loading large set of data and updating a Set immer (proxy) - without autofreeze * 1000: 6977ms immer (proxy) - with autofreeze * 1000: 32ms immer (es5) - without autofreeze * 1000: 35ms immer (es5) - with autofreeze * 1000: 43ms The first case has a median runtime of >200x the other cases, which is unexpectedly poor performance. Expected behavior I would expect that the runtime of the first performance to be closer to that of the other cases. Environment - *Immer version: master* - Occurs with setUseProxies(true) - Occurs with setUseProxies(false) (ES5 only) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#599>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBBC5NUWRAEJU4TO6UTRRKWBPANCNFSM4M7Z4CZQ> .

[srubin]

Yes, I understand that there's a workaround. I still think it's worth addressing. We ran into this unexpected/undocumented slowdown in our own usage of immer, and we'd like to help other users of the library avoid this in the future.

Possible options are:

• Fixing the issue
• Providing better documentation about the performance characteristics of using Map/Set/nested-produce in producers with large objects.
  • A note on the performance page?
  • Providing the multiple-drafts performance test case that I linked to above?
  • A clarification on the Pitfalls page? For example, this note:

Always try to pull produce 'up', for example for (let x of y) produce(base, d => d.push(x)) is exponentially slower than produce(base, d => { for (let x of y) d.push(x)})

does does not apply in my test case. Splitting into two produce calls like this:

const a = produce(baseState, draft => {
    draft.data = dataSet
})
produce(a, draft => {
    draft.data2.add(1)
})

is much more performant.

Thoughts?

[mweststrate]

Added some updates to the docs that will be released soon. Thanks for the suggestions!

[aleclarson]

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀