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

[Converter]: Improve performance and add option to set maximum line length to prevent freezes #64396

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Aug 14, 2022

Fixes #63672

This PR added option to choose maximum checked file size and maximum length of checked line.
By default only files smaller than 4MB are checked and lines shorter than 10000 characters, which seems for me quite reasonable(at the end there is additional info how much exactly lines were ignored).

Additionally due caching regex, searching sometimes can be faster by few percent(I was expecting bigger change, but looks that most of the time is spend on searching files with regex, not creating new regex thousands of times).

When testing projects with a lot of files and short lines, I got a little lower performance than before(this was probably caused by running regexes on lines instead full files like before - this change was required to be able to cache regex), but when checking tps-demo which contains files with longer lines, time to check for renames dropped from 120s to 13s

Edit:
I found with help of hotspot that running regex.sub on small lines is very time consuming, so using string.contains before to decrease amount of regex to run can speedup this part even few times

Some benchmarks:

Project Mode Master branch PR
tps-demo converting 180s 11s
tps-demo validating 116s 11s
jetpaca converting 98s 26s
jetpaca validating 290s 27s
liblast converting 11s 7s
liblast validating 32s 6s

Edit2:
Also added(re-enable actually) check to validate if new function name already exists.

@qarmin qarmin requested a review from a team as a code owner August 14, 2022 15:13
@qarmin qarmin marked this pull request as draft August 14, 2022 15:19
@Chaosus Chaosus added this to the 4.0 milestone Aug 15, 2022
@qarmin qarmin force-pushed the add_line_length_options branch from 40b8fa4 to 96ad63b Compare August 15, 2022 07:18
@qarmin qarmin marked this pull request as ready for review August 15, 2022 07:51
@qarmin qarmin changed the title Add option to set maximum line length to prevent freezes in project converter [Converter]: Add option to set maximum line length to prevent freezes and speedup converting Aug 15, 2022
@qarmin qarmin force-pushed the add_line_length_options branch 2 times, most recently from bb476a3 to 1cabf13 Compare August 15, 2022 18:24
@TokisanGames
Copy link
Contributor

I tried this PR 1cabf13b1fd8bc834bc18e5e69a3b48abeef572a on a fresh copy of my gd3 project. It reports 2011 resources to do.

It is indeed much faster and no longer hangs on the file I mentioned. Great work.

First I tried with 1GB file size and 1MB on one line. It completed in a few minutes.

../../godot/bin/godot.windows.opt.tools.64.exe --convert-3to4 999999 999999
...
Conversion ended - all files(2011), converted files(2011), not converted files(0).
Conversion of all files took 462119 ms.

However, I had many instances of skipped lines. Yet none of them are displayed at the end, neither in summary nor detail. Those are now broken files. It should report the skipped lines files at the end so a user can take steps to fix them.

It even processed my 353MB player scene file with 200 animations, a complex rig, and complete AAA quality human model. 88 seconds to process it is acceptable given what it is.

Trying to convert       1714/2011 file - "assets/models/characters/dorian/Dorian.tscn" with size - 353159 KB
    Checking file took 88459 ms.

If it can process the above file, then it should be able to process any other file I have. The default 10,000 character limit is wayyyyy too low. 999,999 was fine.

Yet it skipped lines > 1MB as I specified.

Trying to convert       329/2011 file - "src/inventory/items/shields/OrnateBuckler.tscn" with size - 1429 KB
    Checking file took 219 ms.
    File was changed, conversion took 219 ms.
    Ignored 1 lines, because their length exceeds maximum allowed characters - 999999

So I expanded it to 100MB which it should be able to allocate and process without sweating. However it hung on a tiny 4mb file. The whole thing could fit in an allocation smaller than many texture files. It's the same structure as described in #63672 with a LOD manger, a few ArrayMeshes.

../../godot/bin/godot.windows.opt.tools.64.exe --convert-3to4 999999 99999999

Trying to convert       361/2010 file - "scenes/BlackMountain.tscn" with size - 4354 KB

# after Ctrl+C
ERROR: Pages in use exist at exit in PagedAllocator: N7Variant5Pools11BucketLargeE
   at: (./core/templates/paged_allocator.h:140)
ERROR: Pages in use exist at exit in PagedAllocator: N7Variant5Pools12BucketMediumE
   at: (./core/templates/paged_allocator.h:140)
ERROR: Pages in use exist at exit in PagedAllocator: N7Variant5Pools11BucketSmallE
   at: (./core/templates/paged_allocator.h:140)
ERROR: BUG: Unreferenced static string to 0: interface_added
   at: unref (core/string/string_name.cpp:131)
ERROR: BUG: Unreferenced static string to 0: bus_layout_changed
   at: unref (core/string/string_name.cpp:131)

I left this for about 10-15 minutes. Allocated memory is only 185mb. Processor is 100% on one core. 350mb Dorian only took 88 seconds and the process allocated up to 3GB.

@qarmin qarmin force-pushed the add_line_length_options branch from 1cabf13 to 4c7c02f Compare August 17, 2022 18:56
@qarmin
Copy link
Contributor Author

qarmin commented Aug 17, 2022

Latest freeze with 4MB file should be fixed now.

After recent changes, String contains method takes ~75% converting time so I don't think that there is room for improvements
The only two possible things which could be done to speedup a little a code:

Screenshot from 2022-08-17 20-55-00

@qarmin qarmin changed the title [Converter]: Add option to set maximum line length to prevent freezes and speedup converting [Converter]: Improve performance and add option to set maximum line length to prevent freezes Aug 17, 2022
@TokisanGames
Copy link
Contributor

Ok, I let it work again on my project w/ 1GB files and 100MB lines:

../../godot/bin/godot.windows.opt.tools.64.exe --convert-3to4 999999 99999999

Trying to convert       570/2010 file - "assets/models/village/buildings/HouseC.tscn" with size - 60322 KB
    Checking file took 30839 ms.
    File was changed, conversion took 30839 ms.

Trying to convert       1614/2010 file - "assets/models/dungeon/architecture/TrippleArch.tscn" with size - 15323 KB
    Checking file took 8674 ms.
    File was changed, conversion took 8674 ms.

Trying to convert       1679/2010 file - "assets/models/creatures/dragon/dragon_body.tres" with size - 13157 KB
    Checking file took 1408 ms.
    File was changed, conversion took 1408 ms.

Trying to convert       1713/2010 file - "assets/models/characters/dorian/Dorian.tscn" with size - 353159 KB
    Checking file took 184101 ms.
    File was changed, conversion took 184101 ms.

Conversion ended - all files(2010), converted files(2010), not converted files(0).
Conversion of all files took 558289 ms.

Nothing was skipped, nothing hung. It chew through even monster files like the highlights included above. Dorian took twice as long, but still acceptable. Great work. Thank you!

@qarmin qarmin force-pushed the add_line_length_options branch 2 times, most recently from a1de7a8 to c60e83f Compare August 18, 2022 17:57
@qarmin
Copy link
Contributor Author

qarmin commented Aug 18, 2022

Recently I only added changes which should speedup searching, especially with big files, so 2x slower checking of 300mb file is quite suspicious.

I think that converting times cannot be really predictable due random OS CPU usage(converter should most of the time use 100% of one core), because one time entire godot demo repository converted most of the time in 32s but sometimes took even in 47s.

The only possible way now to speedup searching is to move computations to multiple cores(which may I implement in future in other PR)

@TokisanGames
Copy link
Contributor

Converting my 4gb project in 10 minutes is fine. The skipping and hanging issues were a bigger deal and those seem resolved.

@qarmin qarmin force-pushed the add_line_length_options branch 5 times, most recently from bea1b91 to 9f00e76 Compare August 27, 2022 10:32
@qarmin qarmin force-pushed the add_line_length_options branch 2 times, most recently from 0eb744e to 2b4d0e6 Compare August 27, 2022 17:59
main/main.cpp Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
@qarmin qarmin force-pushed the add_line_length_options branch from 2b4d0e6 to 3b1259a Compare August 29, 2022 19:26
@akien-mga akien-mga merged commit 992104b into godotengine:master Aug 30, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--convert-3to4 hangs. Also fails on files >500kb
4 participants