Bug 195776 - Reduce the size of Node::deref by eliminating an explicit parentNode check
Summary: Reduce the size of Node::deref by eliminating an explicit parentNode check
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-14 16:02 PDT by Ryosuke Niwa
Modified: 2019-09-06 22:55 PDT (History)
16 users (show)

See Also:


Attachments
Patch (7.59 KB, patch)
2019-03-14 16:33 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (8.14 KB, patch)
2019-03-14 16:58 PDT, Ryosuke Niwa
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.83 MB, application/zip)
2019-03-14 17:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.88 MB, application/zip)
2019-03-14 18:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (2.50 MB, application/zip)
2019-03-14 18:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews204 for win-future (12.88 MB, application/zip)
2019-03-14 21:03 PDT, Build Bot
no flags Details
Fixed the tests (10.94 KB, patch)
2019-03-14 22:16 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed the tests (8.75 KB, patch)
2019-03-14 22:17 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (8.74 KB, patch)
2019-03-14 22:51 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (9.04 KB, patch)
2019-03-15 19:30 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Wait for EWS (9.03 KB, patch)
2019-03-18 11:22 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Address Darin's comments (4.54 KB, patch)
2019-03-29 21:44 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-03-14 16:02:12 PDT
As discussed in https://bugs.webkit.org/show_bug.cgi?id=195661,
we can use the same strategy as String and use the lowest bit of Node::m_refCount
to indicate the pretense of a parent node.

This will reduce the load in Node::def. We can also avoid writing to m_refCount
in the case of calling removedLastRef() as we've done for RefCounted<T> in https://trac.webkit.org/r30042.
Comment 1 Ryosuke Niwa 2019-03-14 16:03:05 PDT
This seems to reduce the binary size of WebCore from 61260928 to 61213824 in my local builds at r242965.
Comment 2 Ryosuke Niwa 2019-03-14 16:33:03 PDT
Created attachment 364715 [details]
Patch
Comment 3 Ryosuke Niwa 2019-03-14 16:58:29 PDT
Created attachment 364721 [details]
Patch
Comment 4 Build Bot 2019-03-14 17:58:19 PDT
Comment on attachment 364721 [details]
Patch

Attachment 364721 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11510518

New failing tests:
fast/repaint/simple-line-layout-shrinking-content.html
accessibility/mac/aria-liveregions-addedelement.html
Comment 5 Build Bot 2019-03-14 17:58:26 PDT
Created attachment 364738 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 6 Ryosuke Niwa 2019-03-14 17:58:42 PDT
Simon and I discussed the idea of using the highest bit instead to avoid having to hard-code 2 everywhere, and in fact I wrote such a patch but it has one downside that the highest bit can also be overridden by the regular ref-counting and we may make m_refCount to 0 in the case of overflow.

Using the lowest bit is slightly more secure as long as the node has a parent, which is more common case anyway. So let's just define a constexpr somewhere for 2 and use the lowest bit.
Comment 7 Build Bot 2019-03-14 18:40:09 PDT
Comment on attachment 364721 [details]
Patch

Attachment 364721 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11511189

New failing tests:
fast/repaint/simple-line-layout-shrinking-content.html
accessibility/mac/aria-liveregions-addedelement.html
Comment 8 Build Bot 2019-03-14 18:40:12 PDT
Created attachment 364748 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 9 Build Bot 2019-03-14 18:59:31 PDT
Comment on attachment 364721 [details]
Patch

Attachment 364721 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11511165

New failing tests:
fast/repaint/simple-line-layout-shrinking-content.html
accessibility/mac/aria-liveregions-addedelement.html
Comment 10 Build Bot 2019-03-14 18:59:34 PDT
Created attachment 364752 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 11 Darin Adler 2019-03-14 19:12:50 PDT
Comment on attachment 364721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364721&action=review

> Source/WebCore/dom/Node.h:723
> -    return m_refCount == 1;
> +    return m_refCount >= 2;

This seems wrong. The point of this is function to return true only if Node has no more than one ref and no less than one ref. Not to return 1 if it has more than one ref. I think the correct implementation is one of these:

    return (m_refCount >> 1) == 1;
    return (m_refCount & ~1) == 2;
Comment 12 Build Bot 2019-03-14 21:03:10 PDT
Comment on attachment 364721 [details]
Patch

Attachment 364721 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11512677

New failing tests:
fast/repaint/simple-line-layout-shrinking-content.html
Comment 13 Build Bot 2019-03-14 21:03:24 PDT
Created attachment 364767 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 14 Ryosuke Niwa 2019-03-14 21:21:31 PDT
Comment on attachment 364721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364721&action=review

>> Source/WebCore/dom/Node.h:723
>> +    return m_refCount >= 2;
> 
> This seems wrong. The point of this is function to return true only if Node has no more than one ref and no less than one ref. Not to return 1 if it has more than one ref. I think the correct implementation is one of these:
> 
>     return (m_refCount >> 1) == 1;
>     return (m_refCount & ~1) == 2;

Oh oops, you're right.

> Source/WebCore/dom/Node.h:728
>      return m_refCount;

This also needs to be fixed.
Comment 15 Ryosuke Niwa 2019-03-14 22:14:28 PDT
Comment on attachment 364721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364721&action=review

>> Source/WebCore/dom/Node.h:728
>>      return m_refCount;
> 
> This also needs to be fixed.

This was the cause of the test failures. With refCount() returning non-zero value,
the optimization in replaceChildrenWithFragment to avoid replacing the text node was disabled.
Comment 16 Ryosuke Niwa 2019-03-14 22:16:00 PDT
Created attachment 364771 [details]
Fixed the tests
Comment 17 Ryosuke Niwa 2019-03-14 22:17:09 PDT
Created attachment 364772 [details]
Fixed the tests
Comment 18 Ryosuke Niwa 2019-03-14 22:37:35 PDT
Comment on attachment 364772 [details]
Fixed the tests

Oh what the heck. I forgot to remove parentNode() check I added back for debugging :(
Comment 19 Ryosuke Niwa 2019-03-14 22:51:52 PDT
Created attachment 364773 [details]
Patch
Comment 20 Geoffrey Garen 2019-03-15 16:35:43 PDT
Comment on attachment 364773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364773&action=review

> Source/WebCore/dom/Node.cpp:2533
> +        m_refCount = 0; // Document needs m_refCount to be 0 here due to m_referencingNodeCount.

I didn't fully understand this comment or the related ChangeLog explanation.

Can we make a nice API so that Document's use of m_refCount works without this unique behavior?

We are calling Document::removedLastRef(). Is that a strong enough signal to the Document that it really has no referencing nodes?

> Source/WebCore/dom/Node.cpp:-2537
> -    m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)

Why don't we need this anymore?

Why does Document still do this if Node doesn't?

> Source/WebCore/dom/Node.h:621
> +    static constexpr uint32_t s_refCountIncrementUnit = 2;

Let's call this s_refCountIncrement to match StringImpl.

> Source/WebCore/dom/Node.h:669
> +    uint32_t m_refCount { s_refCountIncrementUnit };

This probably needs a better name, like m_refCountAndParentBit.

> Source/WebCore/dom/Node.h:730
> +    return m_refCount >> 1;

We probably want an s_refCountShift instead of a hard-coded 1.

> Source/WebCore/dom/Node.h:744
> +    auto refCountWithoutParentBit = m_refCount & ~0x1;

We probably want an s_refCountMask instead of a hard-coded 1.
Comment 21 Ryosuke Niwa 2019-03-15 16:42:24 PDT
(In reply to Geoffrey Garen from comment #20)
> Comment on attachment 364773 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364773&action=review
> 
> > Source/WebCore/dom/Node.cpp:2533
> > +        m_refCount = 0; // Document needs m_refCount to be 0 here due to m_referencingNodeCount.
> 
> I didn't fully understand this comment or the related ChangeLog explanation.
> 
> Can we make a nice API so that Document's use of m_refCount works without
> this unique behavior?

We can't because if m_refCount is not 0 here when m_referencingNodeCount is not zero, Document::removedLastRef doesn't delete document. Instead, it gets deleted when m_referencingNodeCount becomes 0 in decrementReferencingNodeCount. But it deletes Document only when m_refCount is also zero. If we didn't decrement m_refCount here, then we won't be able to tell in decrementReferencingNodeCount whether m_refCount is actually 2, or it would have been reached 0 but we simply didn't decrement.

> We are calling Document::removedLastRef(). Is that a strong enough signal to
> the Document that it really has no referencing nodes?

No. There could be nodes in the document which still references such a Document. This is kept track by m_referencingNodeCount but Document::removedLastRef still needs to run some cleanup steps so that it can remove any Ref/RefPtr Document itself holds.

> > Source/WebCore/dom/Node.cpp:-2537
> > -    m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
> 
> Why don't we need this anymore?

Because we would never decrement m_refCount to 0 in Node::Derek.

> Why does Document still do this if Node doesn't?

Because Document has the issue above. That means we still DO need to decrement m_refCount to 0 when the last de-ref is called so we'd have to set it back to 2 before calling delete.

> > Source/WebCore/dom/Node.h:621
> > +    static constexpr uint32_t s_refCountIncrementUnit = 2;
> 
> Let's call this s_refCountIncrement to match StringImpl.

Sure.

> > Source/WebCore/dom/Node.h:669
> > +    uint32_t m_refCount { s_refCountIncrementUnit };
> 
> This probably needs a better name, like m_refCountAndParentBit.

Will rename.

> > Source/WebCore/dom/Node.h:730
> > +    return m_refCount >> 1;
> 
> We probably want an s_refCountShift instead of a hard-coded 1.

Sure. Will do.

> > Source/WebCore/dom/Node.h:744
> > +    auto refCountWithoutParentBit = m_refCount & ~0x1;
> 
> We probably want an s_refCountMask instead of a hard-coded 1.

Okay, will do.
Comment 22 Geoffrey Garen 2019-03-15 16:59:50 PDT
Comment on attachment 364773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364773&action=review

r=me with comments already discussed, and one new suggestion

>>> Source/WebCore/dom/Node.cpp:2533
>>> +        m_refCount = 0; // Document needs m_refCount to be 0 here due to m_referencingNodeCount.
>> 
>> I didn't fully understand this comment or the related ChangeLog explanation.
>> 
>> Can we make a nice API so that Document's use of m_refCount works without this unique behavior?
>> 
>> We are calling Document::removedLastRef(). Is that a strong enough signal to the Document that it really has no referencing nodes?
> 
> We can't because if m_refCount is not 0 here when m_referencingNodeCount is not zero, Document::removedLastRef doesn't delete document. Instead, it gets deleted when m_referencingNodeCount becomes 0 in decrementReferencingNodeCount. But it deletes Document only when m_refCount is also zero. If we didn't decrement m_refCount here, then we won't be able to tell in decrementReferencingNodeCount whether m_refCount is actually 2, or it would have been reached 0 but we simply didn't decrement.

Would it work to move this assignment of 0 into Document::removedLastRef?

Document::removedLastRef could include a comment that said "Node::removedLastRef doesn't set refCount() to zero because it's not observable. But we need to remember that our refCount reached zero in subsequent calls to decrementReferencingNodeCount()."

How does that sound?
Comment 23 Ryosuke Niwa 2019-03-15 19:11:20 PDT
(In reply to Geoffrey Garen from comment #22)
> Comment on attachment 364773 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364773&action=review
> 
> r=me with comments already discussed, and one new suggestion
> 
> >>> Source/WebCore/dom/Node.cpp:2533
> >>> +        m_refCount = 0; // Document needs m_refCount to be 0 here due to m_referencingNodeCount.
> >> 
> >> I didn't fully understand this comment or the related ChangeLog explanation.
> >> 
> >> Can we make a nice API so that Document's use of m_refCount works without this unique behavior?
> >> 
> >> We are calling Document::removedLastRef(). Is that a strong enough signal to the Document that it really has no referencing nodes?
> > 
> > We can't because if m_refCount is not 0 here when m_referencingNodeCount is not zero, Document::removedLastRef doesn't delete document. Instead, it gets deleted when m_referencingNodeCount becomes 0 in decrementReferencingNodeCount. But it deletes Document only when m_refCount is also zero. If we didn't decrement m_refCount here, then we won't be able to tell in decrementReferencingNodeCount whether m_refCount is actually 2, or it would have been reached 0 but we simply didn't decrement.
> 
> Would it work to move this assignment of 0 into Document::removedLastRef?
> 
> Document::removedLastRef could include a comment that said
> "Node::removedLastRef doesn't set refCount() to zero because it's not
> observable. But we need to remember that our refCount reached zero in
> subsequent calls to decrementReferencingNodeCount()."

Sure, will do.
Comment 24 Ryosuke Niwa 2019-03-15 19:30:56 PDT
Created attachment 364900 [details]
Patch for landing
Comment 25 Ryosuke Niwa 2019-03-15 19:31:15 PDT
Comment on attachment 364900 [details]
Patch for landing

Wait for EWS.
Comment 26 WebKit Commit Bot 2019-03-16 00:50:30 PDT
Comment on attachment 364900 [details]
Patch for landing

Clearing flags on attachment: 364900

Committed r243037: <https://trac.webkit.org/changeset/243037>
Comment 27 WebKit Commit Bot 2019-03-16 00:50:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Radar WebKit Bug Importer 2019-03-16 00:51:30 PDT
<rdar://problem/48950994>
Comment 29 Ryan Haddad 2019-03-18 08:48:06 PDT
This change broke the Windows build:

c:\cygwin\home\buildbot\worker\win10-release\build\source\webcore\dom\node.h(622): error C2220: warning treated as error - no 'object' file generated (compiling source file C:\cygwin\home\buildbot\worker\win10-release\build\Source\WebCore\html\track\TextTrackCueGeneric.cpp) [C:\cygwin\home\buildbot\worker\win10-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
c:\cygwin\home\buildbot\worker\win10-release\build\source\webcore\dom\node.h(622): warning C4245: 'initializing': conversion from 'int' to 'const uint32_t', signed/unsigned mismatch (compiling source file C:\cygwin\home\buildbot\worker\win10-release\build\Source\WebCore\html\track\TextTrackCueGeneric.cpp) [C:\cygwin\home\buildbot\worker\win10-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/2928
Comment 30 Ryan Haddad 2019-03-18 10:06:00 PDT
Reverted r243037 for reason:

Broke the Windows build

Committed r243075: <https://trac.webkit.org/changeset/243075>
Comment 31 Ryosuke Niwa 2019-03-18 11:22:13 PDT
Created attachment 365036 [details]
Wait for EWS
Comment 32 Ryosuke Niwa 2019-03-18 11:22:42 PDT
Why is EWS so insanely slow?
Comment 33 Ryosuke Niwa 2019-03-18 18:32:05 PDT
Comment on attachment 365036 [details]
Wait for EWS

Clearing flags on attachment: 365036

Committed r243122: <https://trac.webkit.org/changeset/243122>
Comment 34 Ryosuke Niwa 2019-03-18 18:32:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Darin Adler 2019-03-26 20:24:48 PDT
Comment on attachment 365036 [details]
Wait for EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=365036&action=review

A few things I noticed after this was landed.

> Source/WebCore/dom/Document.cpp:673
> +        // But we need to remember that our refCount reached zero in subsequent calls to decrementReferencingNodeCount()

No period at the end of this line.

> Source/WebCore/dom/Node.cpp:335
>      // This is a security mitigation in case of programmer errorm (caught by a debug assertion).

"errorm"

A little peculiar that this is not worded the same as the comment with identical purpose in Document.h.

> Source/WebCore/dom/Node.cpp:2527
> +    // This avoids double destruction even when there is a programming error to use Ref<T> / RefPtr<T> on this node.
> +    // There are debug assertions in Node::ref() / Node::deref() to catch such a programming error.
> +    ASSERT(m_refCountAndParentBit == s_refCountIncrement);

Really confusing that this comment claims that an *assertion* avoids double destruction. The correct claim is that this strategy avoids double destruction, but it’s strange to have the comment on an assertion.

> Source/WebCore/dom/Node.h:622
> +    static constexpr uint32_t s_refCountIncrement = 2;
> +    static constexpr uint32_t s_refCountMask = ~static_cast<uint32_t>(0x1);

Kind of funny to use just "2" but then "0x1". Why not just "1"?

Or could write the increment as "1 << 1" and the mask as "~0 << 1" and even consider having a constant for the shift amount of 1, and use that instead of division by s_refCountIncrement.

> Source/WebCore/dom/Node.h:711
> +    auto tempRefCount = m_refCountAndParentBit - s_refCountIncrement;

I think this should be called "updatedRefCount" rather than "tempRefCount".

> Source/WebCore/dom/Node.h:717
> +        return;

This seems more like the place we should have the comment about making double destruction safe, not elsewhere in the functions that make assertions.

> Source/WebCore/dom/Node.h:746
> +    auto refCountWithoutParentBit = m_refCountAndParentBit & s_refCountMask;
> +    m_refCountAndParentBit = refCountWithoutParentBit | !!parent;

I think I would find this more readable either in a single expression or with a much shorter variable name.
Comment 36 Ryosuke Niwa 2019-03-29 21:40:39 PDT
(In reply to Darin Adler from comment #35)
> Comment on attachment 365036 [details]
> Wait for EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365036&action=review
> 
> A few things I noticed after this was landed.
> 
> > Source/WebCore/dom/Document.cpp:673
> > +        // But we need to remember that our refCount reached zero in subsequent calls to decrementReferencingNodeCount()
> 
> No period at the end of this line.

Fixed.

> > Source/WebCore/dom/Node.cpp:335
> >      // This is a security mitigation in case of programmer errorm (caught by a debug assertion).
> 
> "errorm"
>
> A little peculiar that this is not worded the same as the comment with
> identical purpose in Document.h.

I removed this comment in favor of the one in Node.h

> > Source/WebCore/dom/Node.cpp:2527
> > +    // This avoids double destruction even when there is a programming error to use Ref<T> / RefPtr<T> on this node.
> > +    // There are debug assertions in Node::ref() / Node::deref() to catch such a programming error.
> > +    ASSERT(m_refCountAndParentBit == s_refCountIncrement);
> 
> Really confusing that this comment claims that an *assertion* avoids double
> destruction. The correct claim is that this strategy avoids double
> destruction, but it’s strange to have the comment on an assertion.

I removed this comment and added the same one as Document.h in Node.h where we avoid decrementing m_refCountAndParentBit.

> > Source/WebCore/dom/Node.h:622
> > +    static constexpr uint32_t s_refCountIncrement = 2;
> > +    static constexpr uint32_t s_refCountMask = ~static_cast<uint32_t>(0x1);
> 
> Kind of funny to use just "2" but then "0x1". Why not just "1"?

Sure, I'd just use 2 & 1.

> > Source/WebCore/dom/Node.h:711
> > +    auto tempRefCount = m_refCountAndParentBit - s_refCountIncrement;
> 
> I think this should be called "updatedRefCount" rather than "tempRefCount".

Done.

> > Source/WebCore/dom/Node.h:717
> > +        return;
> 
> This seems more like the place we should have the comment about making
> double destruction safe, not elsewhere in the functions that make assertions.

Done that.

> > Source/WebCore/dom/Node.h:746
> > +    auto refCountWithoutParentBit = m_refCountAndParentBit & s_refCountMask;
> > +    m_refCountAndParentBit = refCountWithoutParentBit | !!parent;
> 
> I think I would find this more readable either in a single expression or
> with a much shorter variable name.

Sure, replaced it with a single line expression.
Comment 37 Ryosuke Niwa 2019-03-29 21:44:47 PDT
Reopening to attach new patch.
Comment 38 Ryosuke Niwa 2019-03-29 21:44:48 PDT
Created attachment 366352 [details]
Address Darin's comments
Comment 39 WebKit Commit Bot 2019-03-31 19:03:19 PDT
Comment on attachment 366352 [details]
Address Darin's comments

Clearing flags on attachment: 366352

Committed r243686: <https://trac.webkit.org/changeset/243686>
Comment 40 WebKit Commit Bot 2019-03-31 19:03:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Darin Adler 2019-04-01 08:39:36 PDT
Comment on attachment 366352 [details]
Address Darin's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=366352&action=review

> Source/WebCore/dom/Node.h:621
> +    static constexpr uint32_t s_refCountMask = ~static_cast<uint32_t>(1);

Did you try just ~1 or did that result in a warning?
Comment 42 Ryosuke Niwa 2019-04-01 11:15:26 PDT
(In reply to Darin Adler from comment #41)
> Comment on attachment 366352 [details]
> Address Darin's comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366352&action=review
> 
> > Source/WebCore/dom/Node.h:621
> > +    static constexpr uint32_t s_refCountMask = ~static_cast<uint32_t>(1);
> 
> Did you try just ~1 or did that result in a warning?

That doesn't work on Windows :(