Improve alignment of types
AbandonedPublic

Authored by gladhorn on Jul 25 2018, 6:52 AM.

Details

Reviewers
romangg
Group Reviewers
Plasma

Diff Detail

Repository
R110 KScreen Library
Branch
arcpatch-D14353
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1265
Build 1279: arc lint + arc unit
gladhorn created this revision.Jul 25 2018, 6:52 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 25 2018, 6:52 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gladhorn requested review of this revision.Jul 25 2018, 6:52 AM
romangg requested changes to this revision.Jul 26 2018, 11:23 AM
romangg added a subscriber: romangg.

Why push id down? It should stay at the top imo (or alphabetically). Rest is fine.

This revision now requires changes to proceed.Jul 26 2018, 11:23 AM

Why push id down? It should stay at the top imo (or alphabetically). Rest is fine.

Because of padding. On 64 bit when int is usually 4 bit, there will be a hole of 4 bits in the class, moving it down allows the compiler to pack the class tighter.

Of course it's questionable if this is worth it, it's unlikely that we'll have so many instances of it that it'll ever make a difference.

src/output.cpp
87–90

Actually scale should move up maybe, since it can be 64 bit if it's defined to be double.

gladhorn updated this revision to Diff 38595.Jul 27 2018, 12:59 PM

Move two more members to the right spot

gladhorn updated this revision to Diff 38596.Jul 27 2018, 1:02 PM

initializer order fixed

zzag added a subscriber: zzag.Jul 27 2018, 2:15 PM

I suggest to describe in the summary why data members are ordered from largest to smallest.

In D14353#299282, @zzag wrote:

I suggest to describe in the summary why data members are ordered from largest to smallest.

OK, this should be the default for everyone. To have structures packed tight. Then the compiler can decide on alignment/padding (potentially depending on optimization space vs speed)... Is there any other order for members that makes sense?

zzag added a comment.Jul 27 2018, 2:26 PM

OK, this should be the default for everyone. To have structures packed tight. Then the compiler can decide on alignment/padding (potentially depending on optimization space vs speed)... Is there any other order for members that makes sense?

IIRC padding stuff, not really. Alignment of each data member depends on its size. E.g. if a data member has 8 byte size, then it will be aligned on 8 byte boundaries; if a data member has 4 byte size, then it will be aligned on 4 byte boundaries. Now, if you have uint32_t before uint64_t, there will be 4 byte hole.

Also, I, personally, think this optimization is not worth it. It would matter if we have, say, millions of outputs.

As already said on IRC and also how Vlad sees it: this optimization like for most of our classes is not worth it.

Since we are an open source project, which strives to motivate people to contribute, good readability is much more important. So when ordering the class members we should think of our code as technical documents, whose structure should make it easy for new contributors to learn what the code is doing.

This means that when describing a class general and more important properties should be at the top (e.g. here id, name, type) and more specific or less important ones are at the bottom (e.g. here rotation, scale). Also properties should be grouped if they show similarities (e.g. here pos, size, rotation, scale).

If we have a class in our code where tighter packing really makes sense (much more instances), a comment in the class description should indicate this. Otherwise the default ordering mode should be better readability.

gladhorn abandoned this revision.Jul 31 2018, 2:40 PM

seems like this is not wanted