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

Optimize tracing terminal in sys. #3414

Closed
wants to merge 3 commits into from

Conversation

Redar13
Copy link

@Redar13 Redar13 commented Sep 21, 2024

Does this PR close any issues? If so, link them below.

N/A

Briefly describe the issue(s) fixed.

Makes tracing terminal faster by removing some of the freezes.

@lemz1
Copy link
Contributor

lemz1 commented Sep 21, 2024

wait, how does this make it faster?
wouldn't using mutexes, make it technically slower, since you lock the threads, until the mutex is released?

also im pretty sure the internal printing functions already have a mutex

@EliteMasterEric EliteMasterEric added the status: pending triage The bug or PR has not been reviewed yet. label Sep 22, 2024
@EliteMasterEric
Copy link
Member

I now have the tools to get objective data for this!

I performed the following test code, then filtered the statistics such that I was timing just the trace() call.

  function debug_testTraces():Void
  {
    for (i in 0...10000)
    {
      trace('test $i');
    }
  }

Results before this PR:

image

We see a cumulative time of 423.6ms with a mean of 42.36us and a max of 5.22ms.

Results after this PR:

image

We see a cumulative time of 478.18ms with a mean of 47.82us and a max of 4.14ms.

We also have this time distribution for debug_testTraces as a whole which I forgot to grab for the first test.

image

Conclusion

Reading the code itself, I'm not 100% sure what wrapping the trace call in a mutex would do here. Maybe you misunderstood how multi-threading works? Locking a mutex does NOT create a new thread to do work in, it's for managing work done by threads!

I'm not 100% certain what makes each individual call to trace() take the time it does (probably something related to the lower level system) but overall I found this PR had a slightly negative but overall negligible impact on performance, rather than the positive one this claims to have.

Feel free to reply if you have any objections but for now I will reject this PR.

@EliteMasterEric EliteMasterEric added status: rejected Issue did not pass review or PR cannot be approved. type: optimization Involves a performance issue or a bug which causes lag. and removed status: pending triage The bug or PR has not been reviewed yet. labels Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: rejected Issue did not pass review or PR cannot be approved. type: optimization Involves a performance issue or a bug which causes lag.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants