Trivial performance bug in ObservableMap.replace

[Maaartinus]

I have a:

2. Issue:

• Did you check this issue wasn't filed before?

No behavior change; it's just performance bug.

• State the versions of MobX and relevant libraries. Which browser / node / ... version?

mobx/src/types/observablemap.ts

Line 334 in 6c1d807

const oldKeys = Array.from(this.keys())

I'm proposing the following trivial change:

•        const newKeys = (getMapLikeKeys(values) as any) as K[]
        const oldKeys = Array.from(this.keys())
  

•        const missingKeys = oldKeys.filter(k => newKeys.indexOf(k) === -1)
  

•       const missingKeys = oldKeys.filter(k => !values.has(k))
  

The current behavior has quadratic complexity, with two nested loops (over all old and new keys). So for a map with a few thousands elements it may take ages, while values.has is supposed to be a constant-time operation (leading to linear overall complexity).

[urugator]

Note that values is not neccessarily a Map, it can be object { key: value } or array [[key,value]], so you may need to convert it first:

if (isPlainObject(values)) {
  // could be done in single pass, but whatever
  values = Object.entries(values);
}
if (Array.isArray(values)) {
  values = new Map(values);
} 

However I think we should resolve #1980 first.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically unmarked as stale. Please disregard previous warnings.

[danielkcz]

PR #2057 merged and waiting for the release

[Bnaya]

published

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.