Date:   Thu, 27 Jan 2022 12:27:35 -0800
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     brendanhiggins@...gle.com, linux-kernel@...r.kernel.org,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        skhan@...uxfoundation.org
Subject: Re: [PATCH 3/3] kunit: factor out str constants from binary assertion structs

On Thu, Jan 27, 2022 at 12:21 PM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> On Wed, Jan 26, 2022 at 7:39 PM David Gow <davidgow@...gle.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@...gle.com> wrote:
> > >
> > > If the compiler doesn't optimize them away, each kunit assertion (use of
> > > KUNIT_EXPECT_EQ, etc.) can use 88 bytes of stack space in the worst and
> > > most common case. This has led to compiler warnings and a suggestion
> > > from Linus to move data from the structs into static const's where
> > > possible [1].
> > >
> > > This builds upon [2] which did so for the base struct kunit_assert type.
> > > That only reduced sizeof(struct kunit_binary_assert) from 88 to 64.
> > >
> > > Given these are by far the most commonly used asserts, this patch
> > > factors out the textual representations of the operands and comparator
> > > into another static const, saving 16 more bytes.
> > >
> > > In detail, KUNIT_EXPECT_EQ(test, 2 + 2, 5) yields the following struct
> > >   (struct kunit_binary_assert) {
> > >     .assert = <struct kunit_assert>,
> > >     .operation = "==",
> > >     .left_text = "2 + 2",
> > >     .left_value = 4,
> > >     .right_text = "5",
> > >     .right_value = 5,
> > >   }
> > > After this change
> > >   static const struct kunit_binary_assert_text __text = {
> > >     .operation = "==",
> > >     .left_text = "2 + 2",
> > >     .right_text = "5",
> > >   };
> > >   (struct kunit_binary_assert) {
> > >     .assert = <struct kunit_assert>,
> > >     .text = &__text,
> > >     .left_value = 4,
> > >     .right_value = 5,
> > >   }
> > >
> > > This also DRYs the code a bit more since these str fields were repeated
> > > for the string and pointer versions of kunit_binary_assert.
> > >
> > > Note: we could name the kunit_binary_assert_text fields left/right
> > > instead of left_text/right_text. But that would require changing the
> > > macros a bit since they have args called "left" and "right" which would
> > > be substituted in `.left = #left` as `.2 + 2 = \"2 + 2\"`.
> > >
> > > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
> > > [2] https://lore.kernel.org/linux-kselftest/20220113165931.451305-6-dlatypov@google.com/
> > >
> > > Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> > > ---
> >
> > This definitely _feels_ like it's adding a bit more complexity than
> > would be ideal by splitting this out into a separate function, but I
> > do agree that it's worth it.
>
> I'll note that this was *more* of a simplification until I deduped the
> binary macros.
> Since we only have one macro now passing in the left/right/op strings
> now, it doesn't look like as much of an improvement anymore.
>
> So now the main other benefits are DRYing the assert structs.
> And I lean towards feeling that + stack size decrease = good enough
> reason to go ahead with the refactor.
>
> Re complexity, here's what KUNIT_EXPECT_EQ(test, 1 + 1, 2) turns into
>
> do {
>   typeof(1 + 1) __left = (1 + 1);
>   typeof(2) __right = (2);
>   static const struct kunit_binary_assert_text __text = {
>     .operation = "==",
>     .left_text = "1 + 1",
>     .right_text = "2",
>   };
>   do {
>     if (__builtin_expect(!!(!(__left == __right)), 0)) {
>       static const struct kunit_loc loc = {
>         .file = "lib/kunit/kunit-example-test.c",
>         .line = 29
>       };
>       struct kunit_binary_assert __assertion = {
>         .assert = { .format = kunit_binary_assert_format },
>         .text = &__text,
>         .left_value = __left,
>         .right_value = __right
>       };
>       kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION,
>               &__assertion.assert,
>               ((void *)0));
>     }
>   } while (0);
> } while (0);
>
> Actually, looking at this, I realize we should probably
> 1) move the __text decl into the if statement

Nevermind, was a brainfart.
We can't move that into the if, since that happens inside the
KUNIT_ASSERTION macro and so we need to initialize __text outside of
it.

It's a bit unfortunately we need to pay the cost of initializing
__text even when we might not use it, but that's honestly a fairly
minimal cost and performance isn't KUnit's focus anyways.

> 2) probably should rename loc to __loc, oops.
>
> I'll send out a v2 that does #1.
> Maybe I'll include another patch that does #2 at the end of this
> series since the source patch already got picked up into Shuah's tree.
>
> >
> > I think left_text / right_text are good enough names, too: I wouldn't
> > bother trying to make them .left/.right.
> >
> >
> > Reviewed-by: David Gow <davidgow@...gle.com>

Powered by blists - more mailing lists