EngineHub / CommandHelper

Rapid scripting and command aliases for Minecraft owners
https://methodscript.com
Other
119 stars 71 forks source link

Stacktrace manager optimization #1371

Closed Pieter12345 closed 1 year ago

Pieter12345 commented 1 year ago

Store StacktTraceManager directly in GlobalEnv to prevent a synchronized hashmap lookup on every eval() and seval() call. Move StackTraceManager getter in CClosure to post-environment-clone to ensure that it does not overwrite the StackTraceManager of the original thread when called from a new thread(by for example x_new_thread()).

In the 5 seconds profile below, we can see that a couple of GlobalEnv.GetStackTraceManager() calls take up about 18% CPU time for my specific MethodScript program. afbeelding

PseudoKnight commented 1 year ago

Maybe just do this in clone() instead.

        if(clone.stackTraceManagerThread != Thread.currentThread()) {
            clone.stackTraceManagerThread = Thread.currentThread();
            clone.stackTraceManager = new StackTraceManager();
        }

Then we can have instantiation like:

    private Thread stackTraceManagerThread = Thread.currentThread();
    private StackTraceManager stackTraceManager = new StackTraceManager();

and have GetStackTraceManager() just return stackTraceManager.

In this case, I think moving the order around in CClosure would replace <<main code>> with <<closure>> at the start of a stacktrace on a new thread.

Pieter12345 commented 1 year ago

Maybe just do this in clone() instead.

      if(clone.stackTraceManagerThread != Thread.currentThread()) {
          clone.stackTraceManagerThread = Thread.currentThread();
          clone.stackTraceManager = new StackTraceManager();
      }

Then we can have instantiation like:

  private Thread stackTraceManagerThread = Thread.currentThread();
  private StackTraceManager stackTraceManager = new StackTraceManager();

and have GetStackTraceManager() just return stackTraceManager.

In this case, I think moving the order around in CClosure would replace <<main code>> with <<closure>> at the start of a stacktrace on a new thread.

Doing that does have the benefit of not needing to check two conditions in the getter, but it adds potential issues that will occur when first creating/cloning the environment and then using that environment on a new thread to execute code. I prefer not to require cloning the environment on the new thread to avoid having the wrong stacktrace manager in the environment.

PseudoKnight commented 1 year ago

If that were an issue, it would affect a lot of other things besides the StackTraceManager, and cloning on the new thread is already how things work. So I think it's unnecessary but harmless to approach it like you're doing. It solves the problem.