Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(apiWatch): deep watch should support symbols, performance tweaks #402

Closed
wants to merge 1 commit into from
Closed

fix(apiWatch): deep watch should support symbols, performance tweaks #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions packages/runtime-core/__tests__/apiWatch.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,9 +290,12 @@ describe('api: watch', () => {
})

it('deep', async () => {
const countSymbol = Symbol('countSymbol')

const state = reactive({
nested: {
count: ref(0)
count: ref(0),
[countSymbol]: ref(0)
},
array: [1, 2, 3],
map: new Map([['a', 1], ['b', 2]]),
Expand All @@ -305,6 +308,7 @@ describe('api: watch', () => {
state => {
dummy = [
state.nested.count,
state.nested[countSymbol],
state.array[0],
state.map.get('a'),
state.set.has(1)
Expand All @@ -314,26 +318,30 @@ describe('api: watch', () => {
)

await nextTick()
expect(dummy).toEqual([0, 1, 1, true])
expect(dummy).toEqual([0, 0, 1, 1, true])

state.nested.count++
await nextTick()
expect(dummy).toEqual([1, 1, 1, true])
expect(dummy).toEqual([1, 0, 1, 1, true])

state.nested[countSymbol]++
await nextTick()
expect(dummy).toEqual([1, 1, 1, 1, true])

// nested array mutation
state.array[0] = 2
await nextTick()
expect(dummy).toEqual([1, 2, 1, true])
expect(dummy).toEqual([1, 1, 2, 1, true])

// nested map mutation
state.map.set('a', 2)
await nextTick()
expect(dummy).toEqual([1, 2, 2, true])
expect(dummy).toEqual([1, 1, 2, 2, true])

// nested set mutation
state.set.delete(1)
await nextTick()
expect(dummy).toEqual([1, 2, 2, false])
expect(dummy).toEqual([1, 1, 2, 2, false])
})

it('lazy', async () => {
Expand Down
27 changes: 18 additions & 9 deletions packages/runtime-core/src/apiWatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,17 +234,26 @@ function traverse(value: unknown, seen: Set<unknown> = new Set()) {
traverse(value[i], seen)
}
} else if (value instanceof Map) {
value.forEach((v, key) => {
// to register mutation dep for existing keys
traverse(value.get(key), seen)
})
const keys = [...value.keys()]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cost here is almost the same because it involves spreading the iterator into a plain array. The array spread can often be deceivingly expensive. Not really worth it IMO.

Copy link
Contributor Author

@dsseng dsseng Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested set iterating in some browsers:
Chrome 79.0.3941.4 (64 bit, V8 7.9.293)

image

Firefox 70.0 (64 bit) (мс stands for millisecond in Russian, таймер закончился = timer ended)

image

I ran tests in Node too, but I had to call functions separately since console gets clogged but I don't want to change the task:
Node v12.8.0 (64 bit):

I loaded the same start code:
image

Small set:
image

100 elements:
image
image

Now, 10k elements:
image
image

So, spread method gives better performance than for...of, but (why?) was worse in Chrome and Node with 100-element set

Code:

function forOfSet (val) {
  console.time('for...of')
  console.groupCollapsed('data')
  for (const x of val) {
    console.log(x)
  }
  console.groupEnd()
  console.timeEnd('for...of')
}

function forSpreadSet (val) {
  console.time('for with spread')
  console.groupCollapsed('data')
  const arr = [...val]
  for (let i = 0; i < arr.length; i++) {
    console.log(arr[i])
  }
  console.groupEnd()
  console.timeEnd('for with spread')
}

const set1 = new Set([1, 2, 3])
const set2 = new Set(Array(100).fill(null).map((_, i) => i))
const set3 = new Set(Array(10000).fill(null).map((_, i) => i))

forOfSet(set1)
forSpreadSet(set1)

forOfSet(set2)
forSpreadSet(set2)

forOfSet(set3)
forSpreadSet(set3)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my side I see no significant difference even with a million items in the set. I've looked at both timing and memory performance indicators, refreshing the page for each particular method (so run test with spread, change code to for..of and refresh, a few times over).

Chrome Version 79.0.3945.88 (Official Build) (64-bit) on MacOS Catalina (10.14.6) (spec 2.4GHz i5, 16GB 2133MHz LPDDR3) by launching chrome with:

/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome --enable-precise-memory-info --js-flags="--trace-opt --trace-deopt"

function forOfSet(val, index) {
  console.time('for set' + index)
  const acc = []
  // const arr = [...val]
  for (const x of val) acc.push(x)
  // for (let i=0; i<arr.length; i++) acc.push(arr[i])
  console.timeEnd('for set' + index)
  console.log('memory: total, used, sizeLimit:', window.performance.memory.totalJSHeapSize, window.performance.memory.usedJSHeapSize, window.performance.memory.jsHeapSizeLimit)
  console.log(acc)
}

const set1 = new Set([1,2,3])
const set2 = new Set(Array(100).fill(null).map((_,i) => i))
const set3 = new Set(Array(1000).fill(null).map((_,i) => i))
const set4 = new Set(Array(1000000).fill(null).map((_,i) => i))

// forOfSet(set1, 1)
// forOfSet(set2, 2)
// forOfSet(set3, 3)
forOfSet(set4, 4)

The timing varies with each refresh in a range of 30 to 45 ms, regardless of the loop method being used.

Screen Shot 2019-12-27 at 5 14 45 PM


for (let i = 0; i < keys.length; i++) {
traverse(value.get(keys[i]), seen)
}
} else if (value instanceof Set) {
value.forEach(v => {
traverse(v, seen)
})
const values = [...value.values()]

for (let i = 0; i < values.length; i++) {
traverse(values[i], seen)
}
} else {
for (const key in value) {
traverse(value[key], seen)
const keys = [
...Object.getOwnPropertyNames(value),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this should be the case here. This would iterate over non-enumerable properties too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the technically correct implementation here is using getOwnPropertyDescriptors and then traverse enumerable properties. But I don't know if it's worth it. May want a benchmark to evaluate the perf cost compared to a simple for...in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to make a benchmark just as in upper comment :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for...in seems not to list symbols.

Copy link
Contributor Author

@dsseng dsseng Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark (anything as the first comment):
Chrome:

image

FF:

image

Node:

image
image
image
image
image
image

Results of this benchmark are questionable, and I think both of these should be validated in e.g. a CI env, since my CPU frequency and load isn't really stable.

Code:

function forInObject (val) {
  console.time('for...in')
  console.groupCollapsed('data')
  for (const x in val) {
    console.log(x)
  }
  console.groupEnd()
  console.timeEnd('for...in')
}

function forSpreadSet (val) {
  console.time('for with spread keys')
  console.groupCollapsed('data')
  const arr = [...Object.keys(val)]
  for (let i = 0; i < arr.length; i++) {
    console.log(arr[i])
  }
  console.groupEnd()
  console.timeEnd('for with spread keys')
}

const obj1 = { '1': 1, '2': 2, '3': 3 }
const obj2 = {}
for (let i = 0; i < 100; i++) {
  obj2[i] = i
}
const obj3 = {}
for (let i = 0; i < 10000; i++) {
  obj3[i] = i
}

forInObject(obj1)
forSpreadSet(obj1)

forInObject(obj2)
forSpreadSet(obj2)

forInObject(obj3)
forSpreadSet(obj3)

...Object.getOwnPropertySymbols(value)
]

for (let i = 0; i < keys.length; i++) {
// TS doesn't allow symbol as index type
traverse(value[keys[i] as string], seen)
}
}
return value
Expand Down