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

Use release build config instead of debug for Swift runtime tests (improve performance more than 50%) #3425

Merged
merged 3 commits into from
Dec 23, 2021

Conversation

KvanTTT
Copy link
Member

@KvanTTT KvanTTT commented Dec 21, 2021

No description provided.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 21, 2021

What's wrong with our MaxOSX runner? I see the following error:

Can't find any online and idle self-hosted runner in the current repository, account/organization that matches the required labels: 'self-hosted , macOS , x64'
Waiting for a self-hosted runner to pickup this job...

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 21, 2021

It works after the following changing runs-on: [self-hosted, macOS, x64] -> runs-on: macos-latest

@KvanTTT KvanTTT force-pushed the swift-runtime-tests-optimization branch from afb0f66 to 69b5d8c Compare December 21, 2021 20:01
@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 22, 2021

GitHub CI does not show any improvement in performance but such improvement definitely exists on my local machine. It's weird.

@ericvergnaud
Copy link
Contributor

GitHub CI does not show any improvement in performance but such improvement definitely exists on my local machine. It's weird.

Don't expect any improvement, all MacOS tests run on a single Mac Mini hosted at my home. There are 4 runners configured, which consume 100% CPU since they run test jobs in parallel.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 22, 2021

I changed self-hosted to macos-latest in this pull request because the first one was not working. It looks like macos-latest works slower than self-hosted.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 22, 2021

all MacOS tests run on a single Mac Mini hosted at my home. There are 4 runners configured, which consume 100% CPU since they run test jobs in parallel.

Also, I think we should think about a more reliable and independent server that is not bound to any personal computer.

@ericvergnaud
Copy link
Contributor

all MacOS tests run on a single Mac Mini hosted at my home. There are 4 runners configured, which consume 100% CPU since they run test jobs in parallel.

Also, I think we should think about a more reliable and independent server that is not bound to any personal computer.

It's a matter of cost. Hosted MacOS CI is expensive, unlike Linux which we get for free from Circle CI, and -limited-Windows, which we get for free from AppVeyor. We migrated from Travis CI because they changed their OSS pricing policy.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 22, 2021

Running on MacOS is important because in such a way we cover all platforms (Windows, Linux, Mac), do I understand correctly? Actually, Swift test also can be run on Linux as well (as I've run it on my virtual machine with Ubuntu).

@ericvergnaud
Copy link
Contributor

Swift/Linux is still on Travis because when we migrated to Circle CI I could not get it to work there.
If you can make it work now, great, then we can drop Travis completely.

@ericvergnaud
Copy link
Contributor

Looks like the MacOS runners got killed somehow, I'm currently restarting them

@ericvergnaud
Copy link
Contributor

Running on MacOS is important because in such a way we cover all platforms (Windows, Linux, Mac), do I understand correctly? Actually, Swift test also can be run on Linux as well (as I've run it on my virtual machine with Ubuntu).

Yes it's very important to run Swift and Cpp on MacOS

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 22, 2021

If you can make it work now, great, then we can drop Travis completely.

I'll try. But it looks like Travis is not included in CI now. At least it's not shown on the main page and in the status.

@ericvergnaud
Copy link
Contributor

Looks like your proposed changes crashed the MacOS sessions, I had to cancel all pending jobs in order to restart the CI runners. Please revert all macOS related changes

@ericvergnaud
Copy link
Contributor

If you can make it work now, great, then we can drop Travis completely.

I'll try. But it looks like Travis is not included in CI now. At least it's not shown on the main page and in the status.

We removed it from the page because it was always red dur to lack of credit
But the .travis folder is still there.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 22, 2021

Looks like your proposed changes crashed the MacOS sessions, I had to cancel all pending jobs in order to restart the CI runners. Please revert all macOS related changes

MacOS session was broken 13 days ago: https://github.com/antlr/antlr4/actions?page=5 (last green status was 13 days ago) The current request does not affect self-hosted machine at all because I changed it: https://github.com/antlr/antlr4/pull/3425/files#diff-e38b7572f251b71add05b02e5c33ba21406d09443e45923f1e609e1b1be89e30R11 :

runs-on: macos-latest

Did you mean to revert this line? Also, I don't understand how my MacOS related changes could break something.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 22, 2021

I reverted the configuration back to self-hosted. And now it works similar to my machine and it's more than 50% faster for Swift (or even faster). Compare time for Maven Test in the optimized and in one of the previous versions.

@KvanTTT KvanTTT changed the title Use release build config instead of debug for Swift runtime tests (improve performance up to 50%) Use release build config instead of debug for Swift runtime tests (improve performance more than 50%) Dec 22, 2021
@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 23, 2021

@parrt or @ericvergnaud please merge

@parrt
Copy link
Member

parrt commented Dec 23, 2021

@ericvergnaud I noticed that when I used multiple threads swift-frontend processes exploded on my M1 mac and it was extremely difficult to kill them all before the machine froze. Couldn't even get Shell to run killall.

@parrt
Copy link
Member

parrt commented Dec 23, 2021

@KvanTTT @ericvergnaud I am running and timing Swift on my M1 Mac and then we'll see what this one does.

@parrt
Copy link
Member

parrt commented Dec 23, 2021

Hmmm...if release is always faster, why are we even bothering to allow the debug version? It complicates the code and it's just the testing rig. Would it be reasonable to simply force release to merge a simpler change?

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Dec 23, 2021 via email

@parrt
Copy link
Member

parrt commented Dec 23, 2021

I suspect that there's something weird about M1 + swift + lots of threads combination; we don't see this with other Target so I don't think.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 23, 2021

Hmmm...if release is always faster, why are we even bothering to allow the debug version? It complicates the code and it's just the testing rig. Would it be reasonable to simply force release to merge a simpler change?

It's just a flag, it can be used for debugging in some scenarios. I don't think it complicates anything especially compare to other duplicated code that complicates much more.

@parrt
Copy link
Member

parrt commented Dec 23, 2021

Well my first rule software development is always to use the simplest possible thing that will work. Unless there is a reason to keep debug mode let's remove it and simplify. Thanks!

@parrt parrt added this to the 4.9.4 milestone Dec 23, 2021
@parrt parrt merged commit b6c9d4e into antlr:master Dec 23, 2021
@parrt
Copy link
Member

parrt commented Dec 23, 2021

Thanks @KvanTTT and good catch! Full tests for swift take 42m55.062s single threaded on my machine and I will try again now, although as you pointed out this is testing maybe more of the start of time for each test. The python Version runs superfast so we know that the overall test rig is not the main problem; could be launching of Swift each time or could be the actual parsing part.

@KvanTTT KvanTTT deleted the swift-runtime-tests-optimization branch December 23, 2021 18:44
@parrt
Copy link
Member

parrt commented Dec 23, 2021

Victory for @KvanTTT (Single threaded on my M1 mac):

Before:

[INFO] Tests run: 341, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  42:53 min
[INFO] Finished at: 2021-12-23T10:39:27-08:00
[INFO] ------------------------------------------------------------------------

real	42m55.062s
user	86m40.563s
sys	12m41.397s

AFTER:

real	26m9.591s
user	21m23.939s
sys	4m36.545s

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.

3 participants