# Need help making code more efficient [Solved]

Hi there, I have written a piece of code that basically runs a list of generated operations in order, using this I am generating a mesh 250x250 faces or 62k ish points. Currently it takes about 4 seconds to generate all points and I would like to at least fourth this time if possible
This is the general idea behind how the instructions are generated and what they mean, the first number represents what operation needs to be executed, then the second and third number (if needed) represent the numbers the operation will be executed on. if it says e or elem then it looks for the result from that elements calculation which has been stored in another list

Here is the code that has to run for each point at the moment

``````using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using System.Text.RegularExpressions;
using System;

public class SimplifiedMultiply : MonoBehaviour
{
public List<float> elementValues;
public List<string> calcList;
public int currentIncrement = 0;
public GameObject planeObject;
public float calculatedValue = 0f;
public bool lowerFloat0 = false;

public void FinalCalculation(float xvalue, float yvalue)
{
calcList = new List<string>(planeObject.GetComponent<MathStringConversion>().calculationsList);
if (calcList.Count == 1)
{
if (calcList[0] == "-999999")
{
planeObject.GetComponent<PlaneGenerator>().calculatedValue = float.Parse((xvalue * -1).ToString());
}

else if (calcList[0] == "999999")
{
planeObject.GetComponent<PlaneGenerator>().calculatedValue = float.Parse(xvalue.ToString());
}

else if (calcList[0] == "-888888")
{
planeObject.GetComponent<PlaneGenerator>().calculatedValue = float.Parse((yvalue * -1).ToString());
}

else if (calcList[0] == "888888")
{
planeObject.GetComponent<PlaneGenerator>().calculatedValue = float.Parse(yvalue.ToString());
}

int count = calcList[0].Split(',').Length - 1;
if (count == -1)
{
planeObject.GetComponent<PlaneGenerator>().calculatedValue = float.Parse(calcList[0]);
}
}

for (int i = 0; i < calcList.Count; i++)
{
int count = calcList[i].Split(',').Length - 1;
string[] calcNotifiers = calcList[i].Split(",");
if (count == 1)
{
if (calcNotifiers[1] == "999999" || calcNotifiers[1] == "-999999")
{
if (calcNotifiers[1] == "-999999")
{
calcNotifiers[1] = (xvalue * -1).ToString();
}
else
{
calcNotifiers[1] = xvalue.ToString();
}
}

if (calcNotifiers[1] == "888888" || calcNotifiers[1] == "-888888")
{
if (calcNotifiers[1] == "-888888")
{
calcNotifiers[1] = (yvalue * -1).ToString();
}
else
{
calcNotifiers[1] = yvalue.ToString();
}
}

if (calcNotifiers[0] == "1")
{
string tempString = calcNotifiers[1];
if (tempString.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString = regex.Replace(tempString, "", 1);
tempString = elementValues[int.Parse(tempString) - 1].ToString();
}
float tempfloat = float.Parse(tempString);
float tempFloatFinal = 0f;
tempFloatFinal = SinFunction(tempfloat);
}

else if (calcNotifiers[0] == "2")
{
string tempString = calcNotifiers[1];
if (tempString.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString = regex.Replace(tempString, "", 1);
tempString = elementValues[int.Parse(tempString) - 1].ToString();
}
float tempfloat = float.Parse(tempString);
float tempFloatFinal = 0f;
tempFloatFinal = CosFunction(tempfloat);
}

else if (calcNotifiers[0] == "3")
{
string tempString = calcNotifiers[1];
if (tempString.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString = regex.Replace(tempString, "", 1);
tempString = elementValues[int.Parse(tempString) - 1].ToString();
}
float tempfloat = float.Parse(tempString);
float tempFloatFinal = 0f;
tempFloatFinal = TanFunction(tempfloat);
}

else if (calcNotifiers[0] == "4")
{
string tempString = calcNotifiers[1];
if (tempString.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString = regex.Replace(tempString, "", 1);
tempString = elementValues[int.Parse(tempString) - 1].ToString();
}
float tempfloat = float.Parse(tempString);
float tempFloatFinal = 0f;
tempFloatFinal = AsinFunction(tempfloat);
}

else if (calcNotifiers[0] == "5")
{
string tempString = calcNotifiers[1];
if (tempString.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString = regex.Replace(tempString, "", 1);
tempString = elementValues[int.Parse(tempString) - 1].ToString();
}
float tempfloat = float.Parse(tempString);
float tempFloatFinal = 0f;
tempFloatFinal = AcosFunction(tempfloat);
}

else if (calcNotifiers[0] == "6")
{
string tempString = calcNotifiers[1];
if (tempString.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString = regex.Replace(tempString, "", 1);
tempString = elementValues[int.Parse(tempString) - 1].ToString();
}
float tempfloat = float.Parse(tempString);
float tempFloatFinal = 0f;
tempFloatFinal = AtanFunction(tempfloat);
}
}

else if (count == 0)
{
if (calcNotifiers[0] == "999999" || calcNotifiers[0] == "-999999")
{
if (calcNotifiers[0] == "-999999")
{
}
else
{
}
}

if (calcNotifiers[0] == "888888" || calcNotifiers[0] == "-888888")
{
if (calcNotifiers[0] == "-888888")
{
}
else
{
}
}
}

else
{
if (calcNotifiers[1] == "999999" || calcNotifiers[1] == "-999999")
{
if (calcNotifiers[1] == "-999999")
{
calcNotifiers[1] = (xvalue * -1).ToString();
}
else
{
calcNotifiers[1] = xvalue.ToString();
}
}

if (calcNotifiers[2] == "999999" || calcNotifiers[2] == "-999999")
{
if (calcNotifiers[2] == "-999999")
{
calcNotifiers[2] = (xvalue * -1).ToString();
}
else
{
calcNotifiers[2] = xvalue.ToString();
}
}

if (calcNotifiers[1] == "888888" || calcNotifiers[1] == "-888888")
{
if (calcNotifiers[1] == "-888888")
{
calcNotifiers[1] = (yvalue * -1).ToString();
}
else
{
calcNotifiers[1] = yvalue.ToString();
}
}

if (calcNotifiers[2] == "888888" || calcNotifiers[2] == "-888888")
{
if (calcNotifiers[2] == "-888888")
{
calcNotifiers[2] = (yvalue * -1).ToString();
}
else
{
calcNotifiers[2] = yvalue.ToString();
}
}

if (calcNotifiers[0] == "7")
{
string tempString1 = calcNotifiers[1];
if (tempString1.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString1 = regex.Replace(tempString1, "", 1);
tempString1 = elementValues[int.Parse(tempString1) - 1].ToString();
}
string tempString2 = calcNotifiers[2];
if (tempString2.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString2 = regex.Replace(tempString2, "", 1);
tempString2 = elementValues[int.Parse(tempString2) - 1].ToString();
}
float tempfloat1 = float.Parse(tempString1);
float tempfloat2 = float.Parse(tempString2);
float tempFloatFinal = 0f;
tempFloatFinal = PowerFunction(tempfloat1, tempfloat2);
}

else if (calcNotifiers[0] == "8")
{
string tempString1 = calcNotifiers[1];
if (tempString1.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString1 = regex.Replace(tempString1, "", 1);
tempString1 = elementValues[int.Parse(tempString1) - 1].ToString();
}
string tempString2 = calcNotifiers[2];
if (tempString2.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString2 = regex.Replace(tempString2, "", 1);
tempString2 = elementValues[int.Parse(tempString2) - 1].ToString();
}
float tempfloat1 = float.Parse(tempString1);
float tempfloat2 = float.Parse(tempString2);
float tempFloatFinal = 0f;
tempFloatFinal = MultiplyFunction(tempfloat1, tempfloat2);
}

else if (calcNotifiers[0] == "9")
{
string tempString1 = calcNotifiers[1];
if (tempString1.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString1 = regex.Replace(tempString1, "", 1);
tempString1 = elementValues[int.Parse(tempString1) - 1].ToString();
}
string tempString2 = calcNotifiers[2];
if (tempString2.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString2 = regex.Replace(tempString2, "", 1);
tempString2 = elementValues[int.Parse(tempString2) - 1].ToString();
}
float tempfloat1 = float.Parse(tempString1);
float tempfloat2 = float.Parse(tempString2);
float tempFloatFinal = 0f;
if (tempfloat2 == 0f)
{
lowerFloat0 = true;
}
else
{
tempFloatFinal = DivideFunction(tempfloat1, tempfloat2);
}
}

else if (calcNotifiers[0] == "10")
{
string tempString1 = calcNotifiers[1];
if (tempString1.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString1 = regex.Replace(tempString1, "", 1);
tempString1 = elementValues[int.Parse(tempString1) - 1].ToString();
}
string tempString2 = calcNotifiers[2];
if (tempString2.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString2 = regex.Replace(tempString2, "", 1);
tempString2 = elementValues[int.Parse(tempString2) - 1].ToString();
}
float tempfloat1 = float.Parse(tempString1);
float tempfloat2 = float.Parse(tempString2);
float tempFloatFinal = 0f;
}

else if (calcNotifiers[0] == "11")
{
string tempString1 = calcNotifiers[1];
if (tempString1.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString1 = regex.Replace(tempString1, "", 1);
tempString1 = elementValues[int.Parse(tempString1) - 1].ToString();
}
string tempString2 = calcNotifiers[2];
if (tempString2.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString2 = regex.Replace(tempString2, "", 1);
tempString2 = elementValues[int.Parse(tempString2) - 1].ToString();
}
float tempfloat1 = float.Parse(tempString1);
float tempfloat2 = float.Parse(tempString2);
float tempFloatFinal = 0f;
tempFloatFinal = SubtractFunction(tempfloat1, tempfloat2);
}
}
currentIncrement = currentIncrement + 1;
}
int tempInt = elementValues.Count - 1;
if (lowerFloat0 == true)
{
lowerFloat0 = false;
planeObject.GetComponent<PlaneGenerator>().calculatedValue = 999999999999f;
}

else if (tempInt >= 0)
{
planeObject.GetComponent<PlaneGenerator>().calculatedValue = elementValues[tempInt];
}
elementValues.Clear();
currentIncrement = 0;
}

public float PowerFunction(float number1, float number2)
{
float tempNumber;
if (number1 < 0 && number2 == Mathf.Floor(number2))
{
tempNumber = Mathf.Pow(Mathf.Abs(number1), number2);
if (number2 % 2 == 1)
{
tempNumber = tempNumber * -1;
}
}

else
{
tempNumber = Mathf.Pow(Mathf.Abs(number1), number2);
}
tempNumber = Mathf.Pow(number1, number2);
return (tempNumber);
}

public float MultiplyFunction(float number1, float number2)
{
float tempNumber;
tempNumber = number1 * number2;
return (tempNumber);
}

public float DivideFunction(float number1, float number2)
{
float tempNumber;
tempNumber = number1 / number2;
return (tempNumber);
}

public float AddFunction(float number1, float number2)
{
float tempNumber;
tempNumber = number1 + number2;
return (tempNumber);
}

public float SubtractFunction(float number1, float number2)
{
float tempNumber;
tempNumber = number1 - number2;
return (tempNumber);
}

public float SinFunction(float number1)
{
float tempNumber;
tempNumber = Mathf.Sin(number1);
return (tempNumber);
}

public float CosFunction(float number1)
{
float tempNumber;
tempNumber = Mathf.Cos(number1);
return (tempNumber);
}

public float TanFunction(float number1)
{
float tempNumber;
tempNumber = Mathf.Tan(number1);
return (tempNumber);
}

public float AsinFunction(float number1)
{
float tempNumber;
tempNumber = Mathf.Asin(number1);
return (tempNumber);
}

public float AcosFunction(float number1)
{
float tempNumber;
tempNumber = Mathf.Acos(number1);
return (tempNumber);
}

public float AtanFunction(float number1)
{
float tempNumber;
tempNumber = Mathf.Atan(number1);
return (tempNumber);
}
}
``````

And just for fun this is what the application looks like atm

1 Like

For all performance and optimization issues, ALWAYS start by using the Profiler window:

Window ā Analysis ā Profiler

Generally optimization is:

• avoid doing the thing that is slow
• do it fewer times and store its result
• do the slow thing less frequently over time
• do the slow thing when nobody cares much (eg, during level loading or in a separate thread)
• find a faster way to do the thing (hardest)

Hi, that looks interesting. The code can however clearly be improved using simple optimizations. It is a little hard to mention each one but right off the bat you can see that you set a planeObject which it appears is used for two things neither of which really needs that GO. You Get the MatStringConversion and PlaneGenerator components from it but they could be assigned instead of the planeObject and there would be no need to look them up.

You have a calcList property but I donāt think it needs to be anything but a local variable right? You have a whole lot of āmagic stringsā that represent some state value. Must they be things like ā999999ā and ā-999999ā?

Code such as this: else if (calcNotifiers[0] == ā2ā) means 5 tests that donāt pass if it is a ā6ā.

The following is repeated verbatim regardless of the calcNotifier value

``````string tempString = calcNotifiers[1];
if (tempString.Contains("e"))
{
var regex = new Regex(Regex.Escape("e"));
tempString = regex.Replace(tempString, "", 1);
tempString = elementValues[int.Parse(tempString) - 1].ToString();
}
float tempfloat = float.Parse(tempString);
float tempFloatFinal = 0f;
``````

Set up properly I think you should be able to pass the value to the correct function without all the if statements, The CalcNotifier expresses the match function to call. And those donāt need to be public.

1 Like

Cool I think I figured out the basics of it, but it seems to just be showing me that the update loop is lagging which is where the graph generation is called from(only runs once each time submit is clicked) Is there another part of the window I should look at for specifics, because sadly I cant expand the update loop further to find specifics

Thanks for all the suggestions, I will try implementing them

Thank you so much, just switching the notifiers from the large numbers to x and y made it only take about 1 second

Youāll want to click the āDeep Profileā toggle at the top there, so it will analyse every method call, etc, for you to peruse.

2 Likes

If you donāt need to constantly look for a new generator you should cache these calls.

Your math functions are needlessly verbose, and depending on how the compiler handles the temporary variables creating and working with them may be introducing some performance concerns and allocations. Itās mostly about readability though. Alternatively you can use `{}` and `return` if you prefer that style.

``````public float MultiplyFunction(float number1, float number2) => number1 * number2;
public float DivideFunction(float number1, float number2) => number1 / number2;
public float AddFunction(float number1, float number2) => number1 + number2;
public float SubtractFunction(float number1, float number2) => number1 - number2;
public float SinFunction(float number1) => Mathf.Sin(number1);
public float CosFunction(float number1) => Mathf.Cos(number1);
public float TanFunction(float number1) => Mathf.Tan(number1);
public float AsinFunction(float number1) => Mathf.Asin(number1);
public float AcosFunction(float number1) => Mathf.Acos(number1);
public float AtanFunction(float number1) => Mathf.Atan(number1);
``````

Iām assuming that this is just to remove the `e` in exponential notation.

``````if (tempString.Contains('e'))
{
int index = tempString.IndexOf('e');
if (index != -1)
tempString = tempString.Remove(index, 1);

tempString = elementValues[int.Parse(tempString) - 1].ToString();
}
``````
1 Like

There is more that can be done, I didnāt want to overwhelm you.

1 Like

Once you get something working, the next spiffalicious step is to make the generation iterative.

Letās say you have 250x250 pixelsā¦ first start with 25x25 and produce the mesh, then 50, then 100, then 150, etc., in some increasing step incrementā¦ each time it completes, replace the previous mesh.

The effect would be like web content that loads a low-resolution version, then improves it steadily.

You could display the time taken at each step and have a āstop getting finerā button.

2 Likes

If you want to make it efficient, rewrite that whole thing.
Itāll also become more readable and maintainable along the way.

There are aplenty things going wrong with this code. Hereās a quick rundown:

• Using magic numbers

• More so: using strings (!) representing magic numbers

• Using hardcoded indexes (a form of magic numbers)

• Generally: calculating values and converting them from and ToString() while you go - this is disastrous for both performance and memory usage! You are trashing the garbage collector and even if your output format is string, youād calculate with number types and only afterwards convert the result to string!

• repeated string comparisons

• using regex

• creating a new regex within the loop every time you need it

• doing a āstring containsā test before using the regex rather than letting regex alone give you the corresponding output

• writing separate methods for trivial math functions (sin, cos, etc) and, for no apparent reason, writing it in ANSI C style (could it be that youāre an embedded developer at heart? Then I get where itās coming from. ).

• Said methods are 6 lines each where they could have been a single line each, which would make it very clear how unnecessary these methods are:

• `public float AcosFunction(float number1) => Mathf.Acos(number1);`

• Oh and ā¦ suffixing a method (an object-level function) with āFunctionā

• repetition ā¦ lots of if/else if each of which fundamentally does the exact same thing - here wrapping these blocks into method calls with parameters would make a whole lot more sense!

• a for loop body that spans about 300 lines ā¦ largely because of all that repetition

• not using increment operators ā¦ like x = x + 1 rather than x++

• float.Parse without try/catch => use float.TryParse!

• Generally: absolutely no error checking / error handling. Neither input nor your code will be perfectly flawless.

For this kind of code you better take what youāve learned, and re-implement it from scratch because not only is it extremely inefficient, it is also unmaintainable and hard to debug.

1 Like

I believe there is value in refactoring code over several iterations vs rewriting. Seeing the evolution from āit works but I can barely understand it and I wrote itā to āthis is slickā has real value. Rewriting wonāt happen often but refactoring should happen all the time.

If one can look the following and not sense ācode smellā I donāt think a rewrite will generate improved results.

``````if (calcNotifiers[0] == "999999" || calcNotifiers[0] == "-999999")
{
if (calcNotifiers[0] == "-999999")
{
}
else
{
}
}
``````

No disrespect OP but you need to get a sense of āthis seems way too complexā. It has 2 basic goals, to see if it is a ā999999ā type (weāve already identified we donāt know what that is) and whether it is a negative version of that. In either case the value is used and if it needs to it can use the negative value of it. And it wouldnāt need to multiply by -1 to get the negative.

1 Like

For instance:

``````if (calcNotifiers[0] == "999999" || calcNotifiers[0] == "-999999")
{
}
``````

Well that wonāt quite do it as he doesnāt want a positive xvalue he wants it positive or negative depending upon calcNotifiers[0]. A number of helper methods would go a long way to making this readable (and testable) and fasterā¦

1 Like
``````if (calcList[0] == "-999999")

if (calcNotifiers[0] == "6")
if (calcNotifiers[0] == "999999" || calcNotifiers[0] == "-999999")
``````

These are all indications of code smell. These tests indicate āsomethingā but we never know what. So pass the element in to something that can return an indicator, Like if it indicates āSineFunctionā then return SinePos or SineNeg (perhaps) so all subsequent processing can avoid the array syntax.

There are better ways as well but eliminate the obviously redundant stuff to start.

You can use ProfileMarkerās to breakdown your code in the Profiler. myProfileMarker.Begin(); code to test; myProfileMarker.End()

Unity - Scripting API: Unity.Profiling.ProfilerMarker.ProfilerMarker (unity3d.com)

If like me you prefer to see results in the console (although with less accuracy), you can use the Stopwatch class. myStopwatch.Restart(); code to test; myStopwatch.Stop(); Debug.Log(myStopwatch.elapsedTicks); (or elapsedMilliseconds)

I love math art, you can definitely get this down to less than 0.1ms Iām sure! And on another level you can leverage Burst to achieve even more.

1 Like

Thanks I will try it

Thank you for your suggestions, I will try implementing them Sadly I cant do the last one though, it displays an e then a number for the element that was calculated from before like e1, e4, etc

Thanks I will try using this for testing

Gotcha, makes sense, this bit of code is to replace the negative and positive version with the correct current x and y coordinates I am thinking of calculating the x value 250 times, then the y value 250 times, for where they are used in the function, then calling them from an internal persistent list based on the position in the 250 x 250 matrix, hopefully this will make in more efficient (the first x value will be referenced, then all the z values associated with it, then it will increment the calculated x). I also want to try looking for section where it is only variables being used and precalculate those so it only has to be done once