1632210 - 5x slower than Chrome in Set.values() and Set.delete() when picking elements from a set of integers

Status: NEW

(Core :: JavaScript Engine, task, P5)

Description

[Markus Stange [:mstange]]

Attachment: testcase

This testcase takes ~1500ms in Firefox and ~270ms in Chrome for me. Profile: https://perfht.ml/3eH7mE1

I ran into this while working on symbolication code in the profiler. I was picking and removing elements from a set one-by-one using set.values().next().value and set.delete(val), and a surprisingly large amount of time was spent in set.values(), about 2x of the time spent in set.delete(). Profile: https://perfht.ml/2VrZxKU

I then attempted to make a stand-alone testcase for the problem (see attachment), but in this testcase, delete takes up much more time than values so something must be different. Still, Chrome does a lot better on this testcase than Firefox.

Comment 1

[Jason Orendorff [:jorendorff]]

Unfortunately, I don't think we can prioritize this currently.

For what it's worth, you can delete values from a set while iterating over it, and the behavior is well-defined across browsers.

Changing the function as shown below makes the test case run in 55ms in Firefox, a 27x improvement, and computes the same total. Of course, it depends on the use case...

function sumElements(s)
{
  let sum = 0;
  let iter = s.values();   // <--- new line added
  while (true) {
    const elem = iter.next().value;    // <--- line modified
    if (elem === undefined) {
      break;
    }
    s.delete(elem);
    sum += elem;
  }
  return sum;
}

Comment 2

[Markus Stange [:mstange]]

Oh, that's nice, thanks. (After I ran into the perf issue I also realized that I could vastly simplify the code by just advancing the iterator and not deleting any elements, because I didn't actually need any set membership tests once I got into the state of picking elements.)

Comment 3

[Mayank Bansal]

Chrome: https://share.firefox.dev/4ha0OwG (128ms without the profiler)
Nightly: https://share.firefox.dev/41E6Ykl (800-900ms without the profiler)

Comment 4

[Mayank Bansal]

Nightly: https://share.firefox.dev/4xvT4y5 (~1000ms without the profiler)

Comment 5

[Iain Ireland [:iain]]

We spend 63% of this profile in HashableValueHasher::isEmpty testing for a magic value. This is hot enough that we could consider using isMagicNoReleaseCheck(JS_HASH_KEY_EMPTY) to make the comparison cheaper.

I see a 30% speedup locally with a quick patch.

I'll open a bug.

Comment 6

[Mayank Bansal]

Nightly: https://share.firefox.dev/3V0k8Hr (750ms without the profiler)