1874022 - The PHC check has too much overhead on arm64

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement, P1)

Description

[Markus Stange [:mstange]]

Profile: https://share.firefox.dev/4aOcGCP

https://speedometer-preview.netlify.app/?suite=TodoMVC-Lit-Complex-DOM&iterationCount=100 is spending too much time in MaybePageAlloc, specifically in this check:

https://searchfox.org/mozilla-central/rev/b1a029fadaaabb333d8139f9ec3924a20c0c941f/memory/build/PHC.cpp#1165-1168

int32_t newDelay = GAtomic::DecrementDelay();
if (newDelay != 0) {
  return nullptr;
}

I think the atomic should be revisited. In particular, I don't believe this sentence:

https://searchfox.org/mozilla-central/rev/b1a029fadaaabb333d8139f9ec3924a20c0c941f/memory/build/PHC.cpp#1146-1148

// It's critical that sAllocDelay has ReleaseAcquire semantics, because that
// guarantees that exactly one thread will see sAllocDelay have the value 0.
// (Relaxed semantics wouldn't guarantee that.)

Relaxed atomics are still atomic.

Comment 1

[Paul Bone [:pbone]]

Thanks Markus.

I think we could use thread local variables here rather than atomics.

This bug is almost a duplicate of 1833807, but I'm going to make it "depends on" because once we do that bug we should re-test arm64.

Comment 2

[Paul Bone [:pbone]]

Working on this now.

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 1. Remove the GTls structure r=glandium

Currently we have 3 structures containing mutable data seperated by their
lock-safe behaviour (locked, atomic and TLS). I'd prefer to associate
structures by what they mean. This patch moves the tlsIsDisabled
variable, the only one in GTls into GMutable and removes the GTls
class.

Comment 4

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 2. Move sNow into GMut r=glandium

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 3. Move sAllocDelay into GMut r=glandium

Comment 6

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 9. Reduce the cost of updating sAllocDelay r=glandium

This also means that we can remove the logic that checks if it has wrapped
if PHC was disabled on the current thread since that won't happen due to
other threads. The patch also adds code that has to initialise the delay
on each thread.

Comment 7

[Paul Bone [:pbone]]

Testing the patches first on x86_64 (Ryzen 4 Threadripper) I got the following results for the subtest above (https://speedometer-preview.netlify.app/?suite=TodoMVC-Lit-Complex-DOM&iterationCount=100)

Central: 28.1 +- 0.32
Tip: 28.5 +- 0.36
Disabled: 27.8 +- 0.32

Maybe smaller numbers are better in which case my patch makes this slower?

But looking at the profiles, the overhead of this code goes from 2.2% to 1.5%, so either my patch makes this better or it hits a bottleneck elsewhere.

Central: https://share.firefox.dev/3U4JY9l
Tip: https://share.firefox.dev/4aDsQ0X

I'll run tests on my Mac Mini M1 soon.

Comment 8

[Paul Bone [:pbone]]

On my Mac M1

Central: 38.7 +- 0.40
Tip: 39.2 +- 0.55
PHC disabled: 39.8 +- 0.46

So my patch helps.

Profiles:
Central: https://share.firefox.dev/49n79kA
Tip: https://share.firefox.dev/3vCs45w

it seems to drop from 1.2% to 0.6% here (you have to find pthread_getspecific and pthread_setspecific)

I also noticed that in my "central" profile the branch outside DecrementDelay() showed up with a moderate number of samples, but in the new profile it had zero. So this has improved the cost of MaybePageAlloc(), not only DecrementDelay()

On ARM the atomic used for Now() didn't show up at all. it is Relaxed.

Comment 9

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 5. Move globals to avoid false sharing r=glandium

Comment 10

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 10. maybe_init() uses a single memory access r=glandium

Comment 11

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 12. Inline the hot path from MaybePageAlloc r=glandium

Comment 12

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1874022 - pt 13. Reset the alloc delay even when PHC is disabled

Comment 13

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 14. Seperate arena and non-arena code paths in PHC r=glandium

Comment 14

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 4. Rename the GMut structure and move gMut to a static member r=glandium

Comment 15

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 5. Fix some outdated comments r=glandium

Comment 16

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 6. Rename the GConst structure and move gConst r=glandium

Rename this structure to PHCRegion and move its instance into PHC.

Comment 17

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 7. Move the mutex to a member variable r=glandium

Comment 18

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 8. Add a proof of lock argument to ResetRNG r=glandium

Comment 19

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 11. Improve cache locality r=glandium

This patch uses the wrong approach

Comment 21

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 13. Inline the hot path for free r=glandium

Comment 22

[Paul Bone [:pbone]]

Attachment: Bug 1874022 - pt 15. Apply some autoformatting changes r=glandium

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f2e2a375881
pt 1. Remove the GTls structure r=glandium
https://hg.mozilla.org/integration/autoland/rev/ecae8e56c5cc
pt 2. Move sNow into GMut r=glandium
https://hg.mozilla.org/integration/autoland/rev/d1f91fdc7c68
pt 3. Move sAllocDelay into GMut r=glandium
https://hg.mozilla.org/integration/autoland/rev/6248406a1453
pt 4. Rename the GMut structure and move gMut to a static member r=glandium
https://hg.mozilla.org/integration/autoland/rev/2092fb5bf23b
pt 5. Fix some outdated comments r=glandium
https://hg.mozilla.org/integration/autoland/rev/da3d695e2d65
pt 6. Rename the GConst structure and move gConst r=glandium
https://hg.mozilla.org/integration/autoland/rev/6c5254f3df9f
pt 7. Move the mutex to a member variable r=glandium
https://hg.mozilla.org/integration/autoland/rev/d6539e603aa5
pt 8. Add a proof of lock argument to ResetRNG r=glandium
https://hg.mozilla.org/integration/autoland/rev/41762a317aae
pt 9. Reduce the cost of updating sAllocDelay r=glandium
https://hg.mozilla.org/integration/autoland/rev/6ace59776780
pt 10. maybe_init() uses a single memory access r=glandium
https://hg.mozilla.org/integration/autoland/rev/dc512f63eaa6
pt 11. Improve cache locality r=glandium
https://hg.mozilla.org/integration/autoland/rev/2a81039219c9
pt 12. Inline the hot path from MaybePageAlloc r=glandium
https://hg.mozilla.org/integration/autoland/rev/fa1129aa4279
pt 13. Inline the hot path for free r=glandium
https://hg.mozilla.org/integration/autoland/rev/f72b2832f50f
pt 14. Seperate arena and non-arena code paths in PHC r=glandium

Comment 24

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1f2e2a375881
https://hg.mozilla.org/mozilla-central/rev/ecae8e56c5cc
https://hg.mozilla.org/mozilla-central/rev/d1f91fdc7c68
https://hg.mozilla.org/mozilla-central/rev/6248406a1453
https://hg.mozilla.org/mozilla-central/rev/2092fb5bf23b
https://hg.mozilla.org/mozilla-central/rev/da3d695e2d65
https://hg.mozilla.org/mozilla-central/rev/6c5254f3df9f
https://hg.mozilla.org/mozilla-central/rev/d6539e603aa5
https://hg.mozilla.org/mozilla-central/rev/41762a317aae
https://hg.mozilla.org/mozilla-central/rev/6ace59776780
https://hg.mozilla.org/mozilla-central/rev/dc512f63eaa6
https://hg.mozilla.org/mozilla-central/rev/2a81039219c9
https://hg.mozilla.org/mozilla-central/rev/fa1129aa4279
https://hg.mozilla.org/mozilla-central/rev/f72b2832f50f