[JSC] Make Strong::set cheap

[Constellation]

6b54cb4

[JSC] Make Strong::set cheap
https://bugs.webkit.org/show_bug.cgi?id=241090

Reviewed by Mark Lam.

HandleSet::writeBarrier is frequently called because it is called every time we set a value in Strong< >.
This patch optimizes it,

1. We should make it inline function since it has a super fast path major use can be covered. And this function is small.
2. We should not always remove a node from the list first. We should insert / remove it only when necessary.
3. Remove m_immediateList since it is not necessary.
4. Make HandleNode as a derived class of BasicRawSentinelNode to make implementation simpler.

This change improves promise benchmarks score since promise uses microtasks which hold values via Strong< >.

        ToT
        Time(doxbee-async-bluebird): 42.8 ms.
        Time(doxbee-async-es2017-babel): 36.4 ms.
        Time(doxbee-async-es2017-native): 28.3 ms.
        Time(doxbee-promises-bluebird): 514.2 ms.
        Time(doxbee-promises-es2015-native): 44.8 ms.
        Time(fibonacci-async-es2017-babel): 380.5 ms.
        Time(fibonacci-async-es2017-native): 218.2 ms.
        Time(parallel-async-bluebird): 648.8 ms.
        Time(parallel-async-es2017-babel): 116.9 ms.
        Time(parallel-async-es2017-native): 115.6 ms.
        Time(parallel-promises-bluebird): 638 ms.
        Time(parallel-promises-es2015-native): 82 ms.

        Patched
        Time(doxbee-async-bluebird): 38 ms.
        Time(doxbee-async-es2017-babel): 27 ms.
        Time(doxbee-async-es2017-native): 19.5 ms.
        Time(doxbee-promises-bluebird): 508.3 ms.
        Time(doxbee-promises-es2015-native): 33.3 ms.
        Time(fibonacci-async-es2017-babel): 349.1 ms.
        Time(fibonacci-async-es2017-native): 151 ms.
        Time(parallel-async-bluebird): 639.6 ms.
        Time(parallel-async-es2017-babel): 100.9 ms.
        Time(parallel-async-es2017-native): 101.9 ms.
        Time(parallel-promises-bluebird): 614 ms.
        Time(parallel-promises-es2015-native): 70.9 ms.

* Source/JavaScriptCore/heap/HandleSet.cpp:
(JSC::HandleSet::writeBarrier): Deleted.
* Source/JavaScriptCore/heap/HandleSet.h:
(JSC::HandleSet::heapFor):
(JSC::HandleSet::allocate):
(JSC::HandleSet::deallocate):
(JSC::HandleSet::writeBarrier):
(JSC::HandleSet::toHandle): Deleted.
(JSC::HandleSet::toNode): Deleted.
(JSC::HandleNode::HandleNode): Deleted.
(JSC::HandleNode::setPrev): Deleted.
(JSC::HandleNode::prev): Deleted.
(JSC::HandleNode::setNext): Deleted.
(JSC::HandleNode::next): Deleted.
* Source/JavaScriptCore/heap/Strong.h:
(JSC::Strong::set):

Canonical link: https://commits.webkit.org/251131@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295036 268f45cc-cd09-0410-ab3c-d52691b4dbfc

[MenloDorian]

r=me. I think there's at least one case in HandleSet::writeBarrier below where the isOnList check can be turned into an ASSERT. And if we can strengthen the entry check in HandleSet::writeBarrier, we may be able to remove the other isOnList check too.

[MenloDorian]

Let's see ... the possibilities are:

1. value == empty, *slot == empty: then slot->isCell() is true and value.isCell() is true even though empty != cell. But this is ok because we don't need to so anything if both are empty.

2. value == cell, *slot == cell: this is ok too because we don't need to do anything.

3. value == number, *slot == number: this is ok too because we don't need to do anything.

4. value == cell, *slot == number: in the generic case, the (slot->isCell() == value.isCell()) test will fail, and we won't return early, which is good. In the isCellOnly case, this is impossible.

5. value == cell, *slot == empty: the (!value == !*slot) test will fail, and we won't return early, which is good.

6. value == empty, *slot == cell: the (!value == !*slot) test will fail, and we won't return early, which is good.

7. value == empty, *slot == number: the (!value == !*slot) test will fail, and we won't return early, which is good. However, we'll need to check below if the node is already on list before removing. This case is impossible for the isCellOnly case.

8. value == number, *slot == empty: the (!value == !*slot) test will fail, and we won't return early, which is not great. And we'll need to check below if value is a cell before adding it to m_strongList. This case is impossible for the isCellOnly case.

9. value == number, *slot == cell: in the generic case, the (slot->isCell() == value.isCell()) test will fail, and we won't return early, which is good. However, we'll need to check below if value is a cell before adding it to m_strongList. This case is impossible for the isCellOnly case.

I'm using "number" to represent all the other types of JSValue that are non-cell but are not empty as well.

[MenloDorian]

If we get here, if value is empty, then we know that *slot must have a cell. Hence, we should remove the node from the m_strongList. So, this is correct.

[MenloDorian]

If we get here, then we know that *slot must not have had a cell (because value has a cell). So, we must push the node onto m_strongList. So, this is also correct.

[MenloDorian]

If we get here, value must be a cell. But we know from the entry check that the only way that we can get here is if *slot is either empty or a number. In both case, node should not already be on m_strongList. So, we can turn this if check into an ASSERT.

[Constellation]

Nice, fixed.

[MenloDorian]

If we get here, then we know value is not cell. Because of the entry check, we know that *slot can either be a number of a cell. So, it makes sense that we need this isOnList check. However, if the entry check also ensures that *slot cannot be a number, then we can turn this isOnList check into an ASSERT.

[Constellation]

OK, I've changed the condition,

  150     bool valueIsNonEmptyCell = value && (isCellOnly || value.isCell());
  151     bool slotIsNonEmptyCell = *slot && (isCellOnly || slot->isCell());
  152     if (valueIsNonEmptyCell == slotIsNonEmptyCell)
  153         return;

So, now, after this fast path check, we can make this isOnList() removed :)

[webkit-early-warning-system]

Committed r295036 (251131@main): https://commits.webkit.org/251131@main

Reviewed commits have been landed. Closing PR #1150 and removing active labels.