Make Obj trivial and add a separate ObjCollectionParent type #7402
Conversation
Pull Request Test Coverage Report for Build thomas.goyne_232Details
💛 - Coveralls |
tgoyne
force-pushed
the
tg/obj-perf
branch
3 times, most recently
from
bd61f4b
to
be73d09
Compare
tgoyne
force-pushed
the
tg/obj-perf
branch
2 times, most recently
from
eabaac3
to
d05739a
Compare
|
This PR contains 4 different commits, where only the last one is about making Obj trivial, so perhaps this should have been handled in 4 different commits. I will try to address each commit individually: Less data: Data race: Consolidate ...: ObjCollectionParent: |
We only need the very next path element, not the entire remaining path.
…cated code m_run_time_point was read on the target thread while being written on the worker thread without any locking involved.
Giving Obj a virtual base class turns out to have a meaningful performance impact.
|
I've expanded the tests to include the scenario tested in #7446 plus a few other things. This ended up revealing a pre-existing bug (accessors for collections with depth > 1 are considered not up to date after mutating via that accessor, and so update every time) that I'll fix in a subsequent PR. |
tgoyne
force-pushed
the
tg/obj-perf
branch
2 times, most recently
from
b736962
to
728ba67
Compare
Adding a base class and virtual functions to Obj turned out to have a measurable performance impact due to that it inhibits a lot of optimizations that are possible for trivial types (10-15% in the obj-c read benchmarks, which includes some SDK overhead so the slowdown in the core part is bigger). Moving the implementation of CollectionParent to a separate adaptor type fixes this, but requires some code movement.
I found the logic around when collections need to update difficult to follow and so attempted to simplify it.
update_if_needed()did meaningfully different things on collections and Obj, withObj::update_if_needed_with_status()being equivalent toCollection::update_if_needed(). I renamed these to make Obj match collection and usechecked_update_if_needed()for the version that throws when detached. A lot of the updating logic was duplicated between the various collections, so I consolidated the parts which logically should always be identical.All of the tests passed with clearly incorrect handling of m_parent_version (such as never setting it at all on ObjCollectionParent), which made it hard to tell what it was even needed for. I shifted it to only ever being incremented in Obj and then propagated downwards, which makes it harder to mask bugs. Making it uint32_t cuts the size of Obj by 8 bytes on 64-bit platforms since there's otherwise 7 bytes of trailing padding; it's unclear if this is actually useful.