On Thu, Jan 27, 2022 at 12:21 PM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
>
> On Wed, Jan 26, 2022 at 7:39 PM David Gow <davidgow@xxxxxxxxxx> wrote:
> >
> > On Wed, Jan 26, 2022 at 5:00 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxx/
> > >
> > > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> > > ---
> >
> > 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@xxxxxxxxxx>
