Mobx: Trivial performance bug in ObservableMap.replace

Created on 13 Jun 2019 · 6Comments · Source: mobxjs/mobx

I have a:

[x] Issue:
- [x] Did you check this issue wasn't filed before?

No behavior change; it's just performance bug.

[x] State the versions of MobX and relevant libraries. Which browser / node / ... version?

https://github.com/mobxjs/mobx/blob/6c1d807a44dfc32567f3e55231eb1464a2155826/src/types/observablemap.ts#L334

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).

🍗 enhancement

Source

Maaartinus

Most helpful comment

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.

urugator on 13 Jun 2019

👍2

All 6 comments

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.

urugator on 13 Jun 2019

👍2

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[bot] on 27 Jun 2019

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

stale[bot] on 28 Jun 2019

PR #2057 merged and waiting for the release

FredyC on 25 Jul 2019

👍1

published

Bnaya on 15 Dec 2019

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.