[Mesa-dev] [PATCH] mesa/mtypes: repack gl_texture_object.

[Marek Olšák]

On Wed, Sep 6, 2017 at 4:44 AM, Dave Airlie <airlied at gmail.com> wrote:
> On 6 September 2017 at 03:11, Marek Olšák <maraeo at gmail.com> wrote:
>> On Tue, Sep 5, 2017 at 5:50 PM, Brian Paul <brianp at vmware.com> wrote:
>>> On 09/04/2017 05:29 AM, Marek Olšák wrote:
>>>>
>>>> On Sun, Sep 3, 2017 at 1:18 PM, Dave Airlie <airlied at gmail.com> wrote:
>>>>>
>>>>> From: Dave Airlie <airlied at redhat.com>
>>>>>
>>>>> reduces size from 1144 to 1128.
>>>>>
>>>>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>>>>> ---
>>>>>   src/mesa/main/mtypes.h | 10 +++++-----
>>>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>>>>> index d44897b..3d68a6d 100644
>>>>> --- a/src/mesa/main/mtypes.h
>>>>> +++ b/src/mesa/main/mtypes.h
>>>>> @@ -1012,7 +1012,6 @@ struct gl_texture_object
>>>>>      struct gl_sampler_object Sampler;
>>>>>
>>>>>      GLenum DepthMode;           /**< GL_ARB_depth_texture */
>>>>
>>>>
>>>> The patch looks good, but here are some ideas for future improvements:
>>>>
>>>> GLenum can be uint16_t everywhere, because GL doesn't set higher bits:
>>>>
>>>> typedef uint16_t GLenum16.
>>>> s/GLenum/GLenum16/
>>>>
>>>>> -   bool StencilSampling;       /**< Should we sample stencil instead of
>>>>> depth? */
>>>>>
>>>>>      GLfloat Priority;           /**< in [0,1] */
>>>>>      GLint BaseLevel;            /**< min mipmap level, OpenGL 1.2 */
>>>>> @@ -1033,12 +1032,17 @@ struct gl_texture_object
>>>>>      GLboolean Immutable;        /**< GL_ARB_texture_storage */
>>>>>      GLboolean _IsFloat;         /**< GL_OES_float_texture */
>>>>>      GLboolean _IsHalfFloat;     /**< GL_OES_half_float_texture */
>>>>> +   bool StencilSampling;       /**< Should we sample stencil instead of
>>>>> depth? */
>>>>> +   bool HandleAllocated;       /**< GL_ARB_bindless_texture */
>>>>
>>>>
>>>> All bools can be 1 bit:
>>>>
>>>> bool x:1;
>>>> GLboolean y:1;
>>>>
>>>> etc.
>>>>
>>>>>
>>>>>      GLuint MinLevel;            /**< GL_ARB_texture_view */
>>>>>      GLuint MinLayer;            /**< GL_ARB_texture_view */
>>>>>      GLuint NumLevels;           /**< GL_ARB_texture_view */
>>>>>      GLuint NumLayers;           /**< GL_ARB_texture_view */
>>>>
>>>>
>>>> MinLevel, NumLevels can be ubyte (uint8_t). MinLayer, NumLayers can be
>>>> ushort (uint16_t)... simply by considering the range of possible
>>>> values.
>>>
>>>
>>> There's lots of opportunities along these lines in gl_texture_image. And
>>> since we often have many gl_texture_images per gl_texture_object, and we
>>> often have many textures, it'll probably have considerable impact.  I've
>>> suggested this in the past but never got around to working on it.
>>>
>>> I recall Eric Anholt mentioning a memory profiling tool that was helpful for
>>> finding wasted space in structures, etc.  I don't recall the name right now.
>>> Eric?
>>
>> Dave used pahole for this patch series too. It can't obviously suggest
>> what I suggested above (like changing the types and bits).
>
> Yup this was pahole, doing what Marek describes is definitely something
> that can be done, but needs a lot more care and attention.
>
> Replacing bool with unsigned :1 fields isn't always a win, as you then
> have a mask/shift on the accesses so overall may end up slowing things
> down, and increasing instruction count etc.

bool:1 is better than unsigned:1, because it's has the behavior of
bool. (and the 1-byte alignment might also matter)

Testing bool:1 doesn't need a shift, only a mask, i.e. the compiler should do:
if (structure->somebyte & 0x04)
for testing a bool on the 3rd bit.

The OR operator has the same complexity:
if (structure->bool_val & 0x24)
for testing 2 bools on the 3rd and 6th bit.

And the AND operator:
if ((structure->bool_val & 0x24) == 0x24)

Setting a bool:1 to 1 is done by OR'ing the bit. Setting a bool to 0
is done by AND'ing the bit. No shifts anywhere.

Marek