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

CONFIGURE: Speed up and get rid of little gnomes #2897

Merged
merged 1 commit into from Apr 2, 2021
Merged

Conversation

@orgads
Copy link
Contributor

@orgads orgads commented Mar 29, 2021

Translated most of the engine work from shell to awk.

Tested that the output is the same (except the order of created files).

Time comparison

Windows:

  • Little gnomes - 99s -> 0.3s
  • Creating - 68s -> 1s
  • Total - 205s -> 35s

Linux:

  • Little gnomes - 3s -> 0.03s
  • Creating - 2s -> 0.11s
  • Total - 8s -> 3.8s
@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 29, 2021

By "output" I mean both the stdout and the generated files. All are identical (except the order of "Creating file" lines.

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Mar 29, 2021

I haven't tested it on Linux yet, but I can confirm the performance increase in my MSYS2 environment.

@Mataniko
Copy link
Contributor

@Mataniko Mataniko commented Mar 29, 2021

Very nice!

@aquadran
Copy link
Member

@aquadran aquadran commented Mar 29, 2021

is awk available to all our native systems?

@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 29, 2021

I don't know for sure, but it should be available. It exists on any POSIX system, and it's bundled in busybox.

I tried not to rely on GNU implementation. Tested the script with both gawk and mawk, and it worked good.

@sev- told me on Discord that I can use awk for this.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Mar 29, 2021

You're improving my quality of life, thank you! =)

I tried this on my older mingw (not mingw-w64) environment (Win7) and awk failed on one of the multi-line strings and didn't generate engines.mk.new.

awk: ./engines.awk:332:   print("# This file is automatically generated by configure\n\
awk: ./engines.awk:332:         ^ unterminated string
$ awk --version
GNU Awk 3.1.7
@antoniou79
Copy link
Contributor

@antoniou79 antoniou79 commented Mar 29, 2021

You're improving my quality of life, thank you! =)

I tried this on my older mingw (not mingw-w64) environment (Win7) and awk failed on one of the multi-line strings and didn't generate engines.mk.new.

awk: ./engines.awk:332:   print("# This file is automatically generated by configure\n\
awk: ./engines.awk:332:         ^ unterminated string
$ awk --version
GNU Awk 3.1.7

Looks like it might be an issue with the '#' character in that string. Maybe it should be escaped?

@digitall
Copy link
Member

@digitall digitall commented Mar 29, 2021

@orgads : Mixed feelings about this, mainly re: portability / one more tool needed, but I think relying on awk and sed is fairly reasonable since configure is intended for POSIX toolchains.

One minor point, could you fix the formatting of the new awk script to use TAB indents consistently as per the older shell code. There is a mix of TAB and space identing currently.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Mar 29, 2021

Looks like it might be an issue with the '#' character in that string. Maybe it should be escaped?

I took out the # characters and it failed the same on the same line. Happy to keep testing things, though I've never used awk so keep it simple =)

@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 29, 2021

Probably string continuation is not supported. I'll replace it with multiple prints.

@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 29, 2021

@sluicebox Before I do that, can you check if replacing the \ with " \ and " on the next line works for you?

Something like this:

	print("# This file is automatically generated by configure\n" \
"# DO NOT EDIT MANUALLY\n" \
"# This file is being included by \"Makefile.common\"") > engines_mk
@orgads orgads force-pushed the orgads:no-gnomes branch from 80c1f96 to 9a0c4d6 Mar 29, 2021
@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 29, 2021

@orgads : Mixed feelings about this, mainly re: portability / one more tool needed, but I think relying on awk and sed is fairly reasonable since configure is intended for POSIX toolchains.

I noticed it is already used here and there in the config scripts, but looks like it's only for specific platforms.

One minor point, could you fix the formatting of the new awk script to use TAB indents consistently as per the older shell code. There is a mix of TAB and space identing currently.

Done, and replaced string continuation with line continuation. Hope it fixes the issue with older awk.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Mar 29, 2021

@sluicebox Before I do that, can you check if replacing the \ with " \ and " on the next line works for you?

I tried it, along with what you've updated in this branch which I believe is the same thing, and in both cases get:

awk: ./engines.awk:332:         print("# This file is automatically generated by configure\n" \
awk: ./engines.awk:332:                                                                       ^ backslash not last character on line
@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 29, 2021

Is there a trailing whitespace after the backslash maybe?

I'll try to build old awk and reproduce tomorrow.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Mar 29, 2021

No, and I tried taking that message literally and checking for that, applying the patch several ways, typing it myself, etc

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Mar 29, 2021

It's line endings. If checked out with Windows git client and then built under msys/mingw (at least my version) it breaks; the carriage return is the character it sees as appearing after backslash. I changed engines.awk to unix line endings and it worked. (And it's fast!) Replacing with individual prints sounds like the least-worst workaround.

Update: I just setup a new msys/mingw environment using our wiki instructions to confirm that it has the same problem, and the same gawk version 3.1.7. I wanted to rule out that this only affected environments setup long ago. msys2/mingw-w64 work, and have a much newer gawk (5.1.0 currently)

@orgads orgads force-pushed the orgads:no-gnomes branch from 9a0c4d6 to e23db7f Mar 30, 2021
@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 30, 2021

I pushed a new version with .gitattributes entry to use LF. If you pull it and the file still has CRLF, delete it and checkout.

I'm not sure if it works on old mac though. I think CR is used on it for line endings.

If anyone can test it before merging it would be nice. Otherwise, we can let buildbot do the job.

If it doesn't work, we can always replace these multiline strings with multiple prints. There aren't many of them.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Mar 30, 2021

That fixed it, thanks!

@aquadran
Copy link
Member

@aquadran aquadran commented Mar 30, 2021

is awk supported natively on amigaos4?

@raziel-
Copy link
Contributor

@raziel- raziel- commented Mar 30, 2021

@aquadran

I do have awk on my platform, not sure if it is new enough to be used though
awk version 20121220

@orgads orgads force-pushed the orgads:no-gnomes branch from e23db7f to 27fcc05 Mar 30, 2021
@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 30, 2021

Tested now with nawk 20121220 on linux, and it works fine. The multiline strings had excess newlines, so I changed them back to line continuation instead of string continuation.

Translated most of the engine work from shell to awk.

Tested that the output is the same (except the order of created files).

Time comparison
---------------
Windows:
* Little gnomes - 99s -> 0.3s
* Creating - 68s -> 1s
* Total - 205s -> 35s

Linux:
* Little gnomes - 3s -> 0.03s
* Creating - 2s -> 0.11s
* Total - 8s -> 3.8s
@orgads orgads force-pushed the orgads:no-gnomes branch from 27fcc05 to 83e8ef5 Mar 30, 2021
@orgads
Copy link
Contributor Author

@orgads orgads commented Mar 31, 2021

Any other comments?

@sev-
Copy link
Member

@sev- sev- commented Apr 2, 2021

Thank you, merging, will watch buildbot.

@sev- sev- merged commit 3d13ad1 into scummvm:master Apr 2, 2021
3 checks passed
3 checks passed
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
@sev-
Copy link
Member

@sev- sev- commented Apr 3, 2021

@orgads the following scenario stopped to work for me:

I normally enable only required engines during my development:

./configure --enable-asan --disable-all-engines --enable-engine=testbed,mohawk

Now, in the middle of development I decide to add another engine:

./configure --enable-asan --disable-all-engines --enable-engine=testbed,mohawk,hdb

It nicely compiles only the engine file, but the resulting binary does not contain the engine, only make clean;make fixes it

@orgads
Copy link
Contributor Author

@orgads orgads commented Apr 3, 2021

@sev- Are you sure it's related to this change and not #2874? I tested your scenario, and the generated files are identical after both commands (compared the build directory [without actually building, only configure] after each command with and without this change).

I'll try to fix it anyway.

@orgads orgads deleted the orgads:no-gnomes branch Apr 3, 2021
@orgads
Copy link
Contributor Author

@orgads orgads commented Apr 3, 2021

Tested on linux, and it seems to link after adding the extra engine.

    C++      engines/hdb/window.o
    AR       engines/hdb/libhdb.a
ar: `u' modifier ignored since `D' is the default (see `U')
    RANLIB   engines/hdb/libhdb.a
    C++      base/version.o
    AR       base/libbase.a
ar: `u' modifier ignored since `D' is the default (see `U')
    RANLIB   base/libbase.a
    LINK     scummvm
    DWP      scummvm.dwp

Which platform is it? What exactly doesn't work for you?

@orgads
Copy link
Contributor Author

@orgads orgads commented Apr 3, 2021

Fixed in #2909.

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