KennanChan / Revit.Async

Use task-based asynchronous pattern (TAP) to run Revit API code from any execution context.
MIT License
223 stars 51 forks source link

Reduce CPU usage in the idle event #25

Closed BobbyCJones closed 8 months ago

BobbyCJones commented 8 months ago

Raising idle without delay pegs CPU usage for Revit. I've been running a modified version of Revit.Async without the call to raise without delay since March of 2023, I don't believe this call is necessary.

KennanChan commented 8 months ago

Thanks, I'll review

KennanChan commented 8 months ago

Thanks for pointing out this performance issue!

I just conducted tests using the predefined test command that saves random families to the Desktop folder. Unfortunately, the results were negative.

The purpose of this feature was to address issue #9. It was observed that when the RunAsync() method is called multiple times simultaneously, some of those calls failed to execute if the SetRaiseWithoutDelay() method was not called. The idling event is utilized to release the locker and create the next external event. If the idling event fails to fire immediately, the entire process becomes stuck.

However, calling the SetRaiseWithoutDelay() method without any condition would indeed put unnecessary pressure on the CPU. Perhaps adding a guard before calling SetRaiseWithoutDelay() could be a solution?

I tested the following code, and it appeared to work as expected.

private static void Application_Idling(object sender, IdlingEventArgs e)
{
    if (UnlockKeys.IsEmpty)
    {
       return;
    }
    e.SetRaiseWithoutDelay();
    if (UnlockKeys.TryDequeue(out var unlockKey))
    {
        unlockKey.Dispose();
    }
}
KennanChan commented 8 months ago

We use a lock here to ensure that calls are made in a sequence. It remains a risk if we don't call the SetRaiseWithoutDelay() method under certain conditions, as this may cause the lock to never be released and the RunAsync() method to stop working before Revit restarts. I have a strong feeling about this.

Perhaps we should consider a different approach to ensure that we release the lock safely. A timer maybe?

BobbyCJones commented 8 months ago

Ah, I see. In my applications all RunAsync() calls are initiated by user input, buttons and controls on dockable panes, and are all called singularly. Certainly why I never ran into the multiple simultaneous calls issue. Your potential fix looks good to me. I am very willing to test it, if you'd like. Thanks!

KennanChan commented 8 months ago

I have committed the changes to the feature/SetRaiseWithoutDelay branch, and the PR #26 has been created.