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

Compiling with scu_build=yes takes the double of the time to compile in 4.4 dev 4 #99607

Closed
matheusmdx opened this issue Nov 24, 2024 · 8 comments · Fixed by #99629
Closed

Compiling with scu_build=yes takes the double of the time to compile in 4.4 dev 4 #99607

matheusmdx opened this issue Nov 24, 2024 · 8 comments · Fixed by #99629

Comments

@matheusmdx
Copy link
Contributor

Tested versions

  • Reproducible: 4.4 dev 4 and 5
  • Not reproducible: 4.4 dev 3

System information

Godot v4.4.dev4 - Windows 10.0.19045 - Multi-window, 2 monitors - Vulkan (Forward+) - dedicated AMD Radeon RX 580 2048SP (Advanced Micro Devices, Inc.; 31.0.21921.1000) - AMD Ryzen 5 3600 6-Core Processor (12 threads)2 threads)

Issue description

SCons 4.8.1, Python 3.12, MSVC 2022 17.9.6

While bisecting some issues between 4.4 dev 3 and dev 4 i noticed my compiling time was slower than the normal sometimes and others compiled in the expected time, after some investigation i ended finding that started after pr #98888. Before the cited pr i was able to compile from zero with dev_build=yes scu_build=yes in about 4 minutes, after that pr my compile time jumped to 8:30.

CC @dustdfg

Steps to reproduce

Compile with dev_build=yes scu_build=yes and compare the time before and with pr #98888

Minimal reproduction project (MRP)

N/A

@matheusmdx matheusmdx changed the title scu_build=yes takes the double of the time to compile in 4.4 dev 4 Compiling with scu_build=yes takes the double of the time to compile in 4.4 dev 4 Nov 24, 2024
@lawnjelly
Copy link
Member

To be honest I don't know why this change was made, it is "tinkering", and managed to break the existing implementation (which has been tested multi-platform).

@dustdfg
Copy link
Contributor

dustdfg commented Nov 24, 2024

¯\_(ツ)_/¯ so just revert it. It works the same on linux though

@dustdfg
Copy link
Contributor

dustdfg commented Nov 24, 2024

@anvilfolk just interesting can you reproduce it too? IIRC you said that SCU was broken on windows in some way

@matheusmdx
Copy link
Contributor Author

matheusmdx commented Nov 24, 2024

The thing i noticed is the time to compile without scu is almost the same of using scu after the pr, its looks like the command is not having effect. I also did a test printing the string that section_name holds in methods.py and the amount of paths is the same but they use different slashes, #98888 was using "\" (core\os) while before #98888 was "/" (core/os), so i did a section_name = section_name.replace("\\", "/") and that worked, i know windows can be annoying about the slashes but that makes sense in this case?

@AThousandShips AThousandShips added this to the 4.4 milestone Nov 24, 2024
@AThousandShips AThousandShips moved this from Unassessed to Bad in 4.x Release Blockers Nov 24, 2024
@AThousandShips
Copy link
Member

AThousandShips commented Nov 24, 2024

I think we should try to resolve this rather than just revert it, much more sustainable solution, I will take a look around the code and see

@lawnjelly
Copy link
Member

and the amount of paths is the same but they use different slashes

It will probably be the backslashes or OS specific and not finding files in the hashtable, this is why it was done like this originally (we had to work through a few such issues when it was first added).

@dustdfg
Copy link
Contributor

dustdfg commented Nov 24, 2024

and the amount of paths is the same but they use different slashes

It will probably be the backslashes or OS specific and not finding files in the hashtable, this is why it was done like this originally (we had to work through a few such issues when it was first added).

You mean that it will be in one format in _scu_folders and in another depending on which OS is used? There is ntpath and posixpath python modules which work consistently regardless of host platform

@AThousandShips
Copy link
Member

AThousandShips commented Nov 24, 2024

I can confirm that the engine simply is built without SCU currently, will investigate fixes

Edit: I can also confirm that adding the replace("\\", "/") seems to help, and is done elsewhere, but will need to confirm it works everywhere

dustdfg added a commit to dustdfg/godot that referenced this issue Dec 7, 2024
Followup to godotengine#98888. Note here can't be any regressions like godotengine#99607
because here separator is not OS dependent and set explicitly

Signed-off-by: Yevhen Babiichuk (DustDFG) <dfgdust@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Bad
Development

Successfully merging a pull request may close this issue.

4 participants