D14353: Improve alignment of types

[Frederik Gladhorn]

gladhorn abandoned this revision.
gladhorn added a comment.


  seems like this is not wanted

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

To: gladhorn, #plasma, romangg
Cc: zzag, romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Roman Gilg]

romangg added a comment.


  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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

To: gladhorn, #plasma, romangg
Cc: zzag, romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Vlad Zagorodniy]

zzag added a comment.


  In D14353#299286 , @gladhorn wrote:
  
  > 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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

To: gladhorn, #plasma, romangg
Cc: zzag, romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Frederik Gladhorn]

gladhorn added a comment.


  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?

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

To: gladhorn, #plasma, romangg
Cc: zzag, romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Vlad Zagorodniy]

zzag added a comment.


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

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

To: gladhorn, #plasma, romangg
Cc: zzag, romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Frederik Gladhorn]

gladhorn updated this revision to Diff 38596.
gladhorn added a comment.


  initializer order fixed

REPOSITORY
  R110 KScreen Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14353?vs=38595&id=38596

BRANCH
  arcpatch-D14353

REVISION DETAIL
  https://phabricator.kde.org/D14353

AFFECTED FILES
  src/output.cpp

To: gladhorn, #plasma, romangg
Cc: romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Frederik Gladhorn]

gladhorn updated this revision to Diff 38595.
gladhorn added a comment.


  Move two more members to the right spot

REPOSITORY
  R110 KScreen Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14353?vs=38379&id=38595

BRANCH
  arcpatch-D14353

REVISION DETAIL
  https://phabricator.kde.org/D14353

AFFECTED FILES
  src/output.cpp

To: gladhorn, #plasma, romangg
Cc: romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Frederik Gladhorn]

gladhorn added a comment.


  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.

INLINE COMMENTS

> output.cpp:89
> +int id;
>  qreal scale;
> +Type type;

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

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

To: gladhorn, #plasma, romangg
Cc: romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Frederik Gladhorn]

gladhorn added a comment.


  In D14353#298417 , @romangg wrote:
  
  > 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.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

To: gladhorn, #plasma, romangg
Cc: romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Roman Gilg]

romangg requested changes to this revision.
romangg added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

To: gladhorn, #plasma, romangg
Cc: romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14353: Improve alignment of types

[Frederik Gladhorn]

gladhorn created this revision.
gladhorn added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
gladhorn requested review of this revision.

REPOSITORY
  R110 KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D14353

AFFECTED FILES
  src/output.cpp

To: gladhorn, #plasma
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart