Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logging System - Asserts #47

Open
mbaker3 opened this issue Oct 26, 2020 · 3 comments
Open

Logging System - Asserts #47

mbaker3 opened this issue Oct 26, 2020 · 3 comments
Labels
effort-low Easy - 1 to 2 hours priority-medium Standard task, plan as you see fit. status-todo Tasks ready to be worked on. type-feature New feature or request
Milestone

Comments

@mbaker3
Copy link
Member

mbaker3 commented Oct 26, 2020

Depends on: #33

Taken from a discussion in another repo

When calling Debug.Assert it's helpful to have a message associated with the failed assertion. However that string message is always created and passed into the method whether it triggers or not.

In some cases this is ok, but sometimes generating the error message is expensive, particularly when the assertion is in a hot loop. Finally, the extra garbage generated can get in the way when profiling and distract from actual memory issues.

The suggested solution is to only create assertion messages when the assertion fails. This can be done via a function pointer.

Example
Log.Assert(bool condition, Func<string> messageProvider);

@mbaker3 mbaker3 added effort-low Easy - 1 to 2 hours priority-medium Standard task, plan as you see fit. status-todo Tasks ready to be worked on. type-feature New feature or request labels Oct 26, 2020
@mbaker3 mbaker3 added this to the Anvil v1.0 milestone Oct 26, 2020
@jkeon
Copy link
Member

jkeon commented Nov 30, 2020

An addition to this that I've recently run into is where the Assert check itself is particularly expensive.

Since we already have different levels of logging, it would be great to have levels of Assert as well so that I can toggle the setting to allow for a runthrough where I know performance is going to be abysmal but I'm running some pretty aggressive asserts.

@mbaker3
Copy link
Member Author

mbaker3 commented Aug 18, 2022

Thinking about this some more I don't think there's a way around the string allocation for the assert. You're best off creating tiers that are enabled/disabled with compiler defines. Unless someone has a better idea.

Function Pointer

The function pointer approach can reduce allocations but it's easy to cause allocations:

  • () => $"Some message {instanceVariable}" - No allocation, the compiler just generates a private method in the class and passes the method pointer into the assert.
  • () => $"Some message {localVariable}" - Managed allocation, the compiler generates a class to hold the value. On call it instantiates and populates an instance of that class and passes its method pointer into the assert.
  • `() => "Some message" - One allocation, the compiler generates a class but creates a single static instance of it on first access

So, there are edge case benefits to using the function pointer but I'm not sure it's worth pursuing for the extra effort involved in consuming.

...params

Having the method compose the string for you using ...params wouldn't work either since that will allocate an Array.


The key to making assertion levels work is to keep it easy enough to define specialized compiler defines. I don't think a one size fits all approach will work where we try to define fixed levels of assertion. Maybe we have fixed levels but then projects define their own extension methods with project specific levels.
Ex:

public static class MyProjectLoggerExtension
{
   [Conditional("MYPROJECT_COLLECTIONS_CHECK")]
   public static void Assert_Collections(this Logger, bool condition, string message)
   {
      Logger.Assert(condition, message);
   }
}

@mbaker3
Copy link
Member Author

mbaker3 commented Aug 18, 2022

Thoughts to explore.

Inline null checks.

Logger.Assert(someCondition)?.WithMessage($"Expensive error message {with} {lots} of {expensive} data")

  • If true, Assert would return a single static instance that allowed WithMessage to be called. Otherwise it would return null.

Would want to profile to see if adding a branch would actually be faster.
You would also lose the ability to completely omit the assertions with Conditional attributes.

Assert Yourself

This kind of defeats the purpose but you could do something like:
Logger.Assert(someCondition ? null : $"Expensive error message {with} {lots} of {expensive} data")

  • Assert would only fire a message if the parameter was not null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-low Easy - 1 to 2 hours priority-medium Standard task, plan as you see fit. status-todo Tasks ready to be worked on. type-feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants