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

Stacktrace manager optimization #1371

Merged

Conversation

Pieter12345
Copy link
Contributor

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

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()`).
@PseudoKnight
Copy link
Contributor

PseudoKnight commented Nov 10, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

@Pieter12345 Pieter12345 merged commit 0c54ffd into EngineHub:master Nov 11, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants