Skip to content

perf(query): copy a temporal, an enum or a point by taking its bytes - #5114

Open
Ignition wants to merge 1 commit into
perf/handle-a-value-by-what-it-ownsfrom
perf/copy-a-temporal-by-taking-its-bytes
Open

Ignition wants to merge 1 commit into
perf/handle-a-value-by-what-it-ownsfrom
perf/copy-a-temporal-by-taking-its-bytes

Conversation

@Ignition

Copy link
Copy Markdown
Contributor

A date, a time, a duration, an enum and a point each sit in the value's union whole, owning no
allocation and needing no destructor, but copying one went out of line to a switch over every type a
value can hold. Only a string, a list, a map and the rest that own something take that path now, so
the two questions asked of a type on every value, how to copy it and whether destroying it has
anything to do, collapse into one: does this type own an allocation.

Every type in that group is trivially copyable, so its value is exactly its bytes, and a copy takes
a fixed width of the union rather than choosing a member by type. Choosing by type would make the
copy a jump table over the group, and large enough that the copy constructor and the move assignment
stop being inlined into their callers, which is the only thing that makes asking the question first
worth anything. Taking the bytes is a copy of the value only while every type in the group stays
trivially copyable, which a unit test asserts of each of them by name. No query returns a different
answer.

A date, a time, a duration, an enum and a point each sit in the union whole, owning no allocation
and needing no destructor, but copying one went out of line to a switch over every type a value can
hold. Only a string, a list, a map and the rest that own something take that path now, so the two
questions asked of a type on every value, how to copy it and whether destroying it has anything to
do, collapse into one: does this type own an allocation.

Every type in that group is trivially copyable, so its value is exactly its bytes, and a copy takes
a fixed width of the union rather than choosing a member by type. Choosing by type would make the
copy a jump table over the group and large enough that the copy constructor and the move assignment
stop being inlined into their callers, which is the only thing that makes asking the question first
worth anything. Taking the bytes is a copy of the value only while every type in the group stays
trivially copyable, which a unit test now asserts of each of them by name.
@Ignition Ignition added this to the mg-v3.14.0 milestone Oct 12, 2026
@Ignition Ignition added Capability - speed or performance CI -build=community -test=core Run community build and core tests on push CI -build=coverage -test=core Run coverage build and core tests on push CI -build=debug -test=core Run debug build and core tests on push CI -build=debug -test=integration Run debug build and integration tests on push CI -build=release -test=core Run release build and core tests on push CI -build=release -test=e2e Run release build and e2e tests on push CI -build=release -test=benchmark Run release build and benchmark on push CI -build=coverage -test=clang_tidy labels Oct 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has potential conflicts with the following other open pull requests which modify the same files:

@Ignition

Copy link
Copy Markdown
Contributor Author

Tracking

Standard development

CI Testing Labels

  • Select the appropriate CI test labels (CI -build=build-name -test=test-suite)

Documentation checklist

  • Add the documentation label
  • Add the bug / feature label
  • Add the milestone for which this feature is intended
    • If not known, set for a later milestone
  • Write a release note, including added/changed clauses
  • [ Documentation PR link memgraph/documentation#XXXX ]
    • Is back linked to this development PR

@Ignition

Copy link
Copy Markdown
Contributor Author

Some notes for review.

What the code does now. TypedValue::Type is ordered so that every type owning no allocation
comes first, and OwnsAnAllocation is a single comparison against the enumerator that ends the
group. The copy constructor, the move assignment and the destructor each ask it once. A type that
owns nothing is copied by taking a fixed width of the union; everything else goes to the existing
out-of-line path, which is unchanged. This pull request moves the temporals, the enum and the two
point types from the second group into the first.

Why the copy takes bytes rather than choosing a union member by type. The whole value of asking
the question first is that the answer stays at the call site, and expression evaluation builds and
destroys a value per node per row. A switch over the group would make the copy a jump table, and
would grow the copy constructor and the move assignment past the point where the inliner keeps them
in their callers. At that point the fast path becomes a function call and costs more than the
out-of-line switch it replaced. A fixed-width copy is branchless and small enough to inline, which
is why it is the form here.

Why memcpy specifically, since raw byte copying deserves a second look. Since C++20 memcpy
is specified to implicitly create objects in the destination, so after the copy there is a real
object of the copied type at that address rather than a punned one. Reading the source through the
union is sound because the aliasing rules let std::byte alias any object, and copying the object
representation of a trivially copyable type and back is defined to reproduce the value. std::copy
would compile to the same instructions but assigns element by element with none of that guarantee,
so it is the weaker form despite reading as the more modern one. The eight static_asserts on
trivial copyability are what makes any of this hold, which is why they sit beside the list rather
than in the header.

The one thing a reader should check is the width: a type added to the first group without being
named in the width calculation would be copied short. That fails the existing copy-construction
coverage, which copies every shape of every type and compares, rather than passing silently.

Verified. Unit and property-based suites covering the value, its shapes, the four value
relations, expression evaluation, comparable bounds and the aggregate operators: all green.

Measured as instructions retired, both arms built from one tree and run interleaved on a pinned
core against this pull request's base branch. Across the aggregation benchmark the geometric mean is
about half a percent fewer instructions, with twenty of the twenty-one cases whose run to run spread
is small enough to read improving. That understates the change for anything temporal, since those
cases are mostly numbers and strings: copying or destroying a date costs about a third of what it
did, and building and tearing one down is down by about the same. Copying a list improves too,
because the smaller inline path leaves more room for the rest. A short string is unchanged.

@Ignition
Ignition added this pull request to stack #5115 October 12, 2026 14:17
@Ignition
Ignition requested a review from imilinovic October 12, 2026 14:17
@Ignition Ignition self-assigned this Oct 12, 2026
@sonarqubecloud

Copy link
Copy Markdown

@Ignition
Ignition marked this pull request as ready for review October 12, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Capability - speed or performance CI -build=community -test=core Run community build and core tests on push CI -build=coverage -test=clang_tidy CI -build=coverage -test=core Run coverage build and core tests on push CI -build=debug -test=core Run debug build and core tests on push CI -build=debug -test=integration Run debug build and integration tests on push CI -build=release -test=benchmark Run release build and benchmark on push CI -build=release -test=core Run release build and core tests on push CI -build=release -test=e2e Run release build and e2e tests on push

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant