it and describe space leaks

[guibou]

The implementation for it and describe can space leak thunk and String.

Roughly speaking, both function takes the String for the name and convert it in a lazy way into a Text stored in the Specify or Describe node.

However this value is only evaluated when the name of the it or describe is required.

In a context where we dynamically build the test name (e.g. it ("testCase" <> foo)) it means that it maintains a thunk to the dynamic expression which can actually maintain alive a lot of objects.

In a context were i have multiple thousand tests with a dynamic name, I observed up to a GiB of retained data which should not be there and this had a negative impact on the test suite performance (e.g. GC was painfully walking all of these values). Using the following workaround removed these GiB (replacing them by more compact Text reference) and reduce the runtime of my test suite by up to 2x.

Workaround

One workaround for user are to force to WHNF the name before calling the it. In this context, it will still leak references to not compact String, but not to other object.
Another workaround is to fore the name to a compact data (e.g. Text) and then convert it to String. The GC will only have to track a thunk and the Text. For example:

describe "hello" $ do
   let !name = "test" <> f x

  it (unpack name) $ ...

Solution

The Text value inside the Scecify or Describe node should be forced in the it or describe implementation.

I'll submit an MR soon, but I'm opening this issue for reference.

[NorfairKing]

Would it be a good idea to instead make the Text inside of the DefSpecifyNode strict?
I guess that would be wasteful if the node is never needed, but I'm not sure that's something worth worrying about...

[NorfairKing]

In fact, maybe every value in the SpecDefTree` should be strict ... 🤔

[NorfairKing]

If you think that's a better solution, I'll implement it myself and credit you. You've done enough work here :)

[guibou]

Would it be a good idea to instead make the Text inside of the DefSpecifyNode strict?

That's semantically the same as my MR, I guess, but ensures that it is forced everywhere.

I guess that would be wasteful if the node is never needed, but I'm not sure that's something worth worrying about...

I'm sure that it is not worth worrying about ;) I mean, if really really your test suite performance depends on the time spent to evaluate the name of tests that you are filtering out, well, you and I have different problems ;) On the other hand, I'm sure that leaking String (and maybe other things) is a problem ;) (For the record, one of my testsuite loads the tests from a directory containing thousands of reference test case, which are SBML files and name the test based on the content of the file (e.g. name of the test case + some tags which helps me to later filter my testsuite based on some tags. Unfortunately, the lazyness was retaining the complete content of the reference file ;)

(side note: there is a "similar" problem with golden test reference which are FilePath and which hence always retain at least a reference to a [Char] or even worse).

In fact, maybe every value in the SpecDefTree` should be strict ... 🤔

As a rule of thumb, I'm really in favor of "every field strict unless you have a good reason". I kinda like StrictData actually.

If you think that's a better solution, I'll implement it myself and credit you. You've done enough work here :)

Strict fields on the different fields is a better solution indeed.