1949087 - Remove the atomic from mLastSignificantReuse

Status: NEW

(Core :: Memory Allocator, task)

Description

[Paul Bone [:pbone]]

Bug 1920451 introduces a new variable in each arena for the last time the arena saw sagnificant memory reuse (a dirty page being allocated). The timestamp is an atomic since it's read without taking the arena's lock. In jstutte's experiments he found that SequentiallyConsistent performed better than Relaxed and his hypothesis was that the memory is updated "sooner".

Another option is to write the state within the arena's lock which uses the locks' semantics to provide sequentially consistent memory ordering for no additional cost (it uses the lock's memory ordering on release). This means that the variable doesn't need to be atomic and may even be updated sooner.

If we don't need atomics on the time anymore we can probably use std::chrono's data types.

Comment 1

[Jens Stutte [:jstutte]]

(In reply to Paul Bone [:pbone] from comment #0)

Bug 1920451 introduces a new variable in each arena for the last time the arena saw sagnificant memory reuse (a dirty page being allocated). The timestamp is an atomic since it's read without taking the arena's lock. In jstutte's experiments he found that SequentiallyConsistent performed better than Relaxed and his hypothesis was that the memory is updated "sooner".

To expand a bit on that: The main thread reads the timestamp to decide when to purge, and several threads update that timestamp. If the store made on a different thread is not yet visible on the main thread's load, we miss a significant reuse signal and may just purge earlier, resulting in less performance gains from bug 1920451.

Playing around with compiler explorer seems to confirm, that a plain store on x86-64 will not use xchg unless sequentially consistent is used.

Another option is to write the state within the arena's lock which uses the locks' semantics to provide sequentially consistent memory ordering for no additional cost (it uses the lock's memory ordering on release). This means that the variable doesn't need to be atomic and may even be updated sooner.

We probably do not want to take the arena's lock on read, I assume. So we would need to still guarantee the integrity of the read/write, that is use a relaxed atomic, to be safe on all platforms? Also to make tsan happy? Writing a relaxed atomic inside the lock might be doing what we want, I guess.

If we don't need atomics on the time anymore we can probably use std::chrono's data types.

We could also do bug 1948601 for that.