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

Performance improvements #199

Merged
merged 16 commits into from
Oct 27, 2017
Merged

Performance improvements #199

merged 16 commits into from
Oct 27, 2017

Conversation

maddinat0r
Copy link
Contributor

This PR improves overall compile speed by a factor of ~10.
Special thanks to @Daniel-Cortez for several changes.

@Daniel-Cortez
Copy link
Contributor

Daniel-Cortez commented Oct 22, 2017

The initial size of the hashmap still probably needs to be decreased, though.
IMHO, even if the hashmap growth process (memory reallocation, rehashing) takes time, there's no real need to pre-allocate 8 million (!) slots for every compiled script. The biggest SA-MP gamemode script I tried to compile with this made the compiler use roughly 4k slots at most.

@Southclaws
Copy link
Collaborator

This build appears to break argument-defined constants:

#if defined SOME_CONST
#error defined
#else
#error not defined
#endif

original:

$ pawncc.exe test.pwn SOME_CONST=
Pawn compiler 3.10.2                    Copyright (c) 1997-2006, ITB CompuPhase

test.pwn(2) : fatal error 111: user error: defined

This PR:

$ pawncc.exe test.pwn SOME_CONST=
Pawn compiler 3.10.2                    Copyright (c) 1997-2006, ITB CompuPhase

test.pwn(4) : fatal error 111: user error: not defined

@maddinat0r
Copy link
Contributor Author

I can't reproduce this with this PR rebased against 3.10.3. There probably was a bug before 3.10.3 somewhere related to your issue.

@Zeex
Copy link
Contributor

Zeex commented Oct 24, 2017

That is a huge improvement in performance! But I think it needs more testing before getting merged as it potentially could break a lot of things.

Unfortunately I can't test it as I don't do any scripting at big enough scale besides small test scripts, but I can create a separate release for this version and have other people download and test it.


configuration:
- RelWithDebInfo

before_build:
- cmake -G "Visual Studio 10 2010" source/compiler -DCPACK_GENERATOR=ZIP
- cmake -G "Visual Studio 14 2015" source/compiler -DCPACK_GENERATOR=ZIP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for updating to 2015? Visual Studio 2010 runtime was more widespread when this file was originally created so it was used to avoid troubles with missing MSVC runtime DLLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AppVeyor build failed because the new hashmap depends on stdbool.h, and VS 2010 didn't ship it. But maybe VS 2012 also has it, I didn't look into that.

@Zeex
Copy link
Contributor

Zeex commented Oct 24, 2017

I created a new download here: https://github.com/Zeex/pawn/releases/tag/v3.10.3-performance

@Y-Less
Copy link
Member

Y-Less commented Oct 24, 2017

I ran the full YSI test suite (on my broken mid-edit local copy) and didn't have any new issues/fails, so that's reassuring to me at least.

@spacemud
Copy link
Contributor

Reporting in: my fairly large mode (probably similar to SS in scale) compiles and runs fine with this fork.

@Y-Less
Copy link
Member

Y-Less commented Oct 24, 2017

I was intending to test SS as well, but maybe @Southclaws can weigh in?

Edit: Just noticed he is already in this thread. Oh well...

@Dayvison
Copy link

Compiled well here, amx have no change
36 seconds to 2.6 xD

@SysadminJeroen
Copy link
Contributor

I have upgraded my continous intergration with zeex:performance (a.k.a. v3.10.3-performance).

This is the Dockerfile I'm using.

FROM centos:7

MAINTAINER [email protected]

ENV VERSION 3.10.3-performance
ENV PAWNCFOLDER ~/pawn

LABEL	name="Zeex's pawn compiler" \
				vendor="Zeex" \
				license="zLib/libpng" \
				build-date="20171025"

RUN yum -y install \
				deltarpm epel-release; \
		yum -y groupinstall \
				'Development Tools'; \
		yum -y install \
				kernel-devel git glibc.i686 \
				libuuid libuuid-devel openssl \
				openssl-devel wget libgcc.i686 \
				gcc gcc-c++ cmake28 \
				cmake make glibc-devel.i686 \
				libgcc.i686 libstdc++-devel.i686; \
		yum -y clean \
				all

RUN git clone https://github.com/Zeex/pawn.git $PAWNCFOLDER; \
		cd $PAWNCFOLDER; \
		git checkout -b performance origin/performance; \
		mkdir build && cd build; \
		cmake ../source/compiler -DCMAKE_C_FLAGS=-m32 -DCMAKE_BUILD_TYPE=Release; \
		make; \
		mv libpawnc.so /lib/; \
		mv pawndisasm /bin/; \
		mv pawncc /bin/; \
		cd ~; \
		rm -rf $PAWNCFOLDER/;

Furthermore, these are the results using time pre-v3.10.3-performance.

real	1m39.890s
user	1m39.120s
sys	0m0.160s
real	1m52.975s
user	1m51.816s
sys	0m0.184s
real	1m51.449s
user	1m50.204s
sys	0m0.240s

These are the results using time with v3.10.3-performance.

real	0m9.082s
user	0m8.688s
sys	0m0.240s
real	0m10.315s
user	0m9.916s
sys	0m0.244s
real	0m9.977s
user	0m9.444s
sys	0m0.360s

Here is a screenshot of the continuous integration page of the latest builds, I think that the numbers speak for themselves.
chrome_2017-10-25_01-38-38

I will test if this pull request broke my gamemode, however I doubt it.

@ziggi
Copy link
Contributor

ziggi commented Oct 25, 2017

Tested with Open-GTO gamemode and everything works fine. Vote for merge it! 💥

Results on my i5-7500:

  • Old time: 19.6s
  • New time: 2.7s

7x speed improvement, this is awesome!:clap:

@spacemud
Copy link
Contributor

This version seems to omit the preceding character in strings when the line continuation operator is used:

#include <a_samp>

main()
{
    new
        string[64] = 
        "\
        this is a \
        multi line \
        string \
        yay \
        ";

    print(string);

    return 1;
}

On 3.10.2 this gives:

this is a multi line string yay

...as expected.
On 3.10.3:

this is amulti linestringyay

I noticed this after getting errors from the MySQL plugin - all my multi line queries were getting mangled!

@Zeex
Copy link
Contributor

Zeex commented Oct 25, 2017

@spacemud I think it's because of another patch that I did in 3.10.3, i.e. no this PR. I'll look into it.

@Zeex Zeex merged commit 08f760f into pawn-lang:master Oct 27, 2017
@SysadminJeroen
Copy link
Contributor

Partay.

@VIRUXE
Copy link

VIRUXE commented Oct 28, 2017

It's working with Scavenge and Survive. But still it's a 6sec diff from the russian one.

@maddinat0r maddinat0r deleted the performance branch October 28, 2017 20:13
@sondrekje
Copy link

I'm not quite sure which version my previous compiler was, but the compiler time suddenly started taking around 5 minutes. After switching to this compiler however, it usually compiles within a minute. It is a very large gamemode though.

Anyhow, I noticed one thing which might be a bug after updating the compiler:

) ON DUPLICATE KEY UPDATE \ donation_jailcards = %d,\ donation_fireworks_sm = %d,\

Would for instance work perfectly with the previous compiler, but after updating it it would remove the space between UPDATE and donation_jailcards, I even tried adding several spaces after update and before donation_jailcards to see if that would make any difference, but it did not.

I did however fix my problem by doing this:

) ON DUPLICATE KEY UPDATE donation_jailcards = %d,\ donation_fireworks_sm = %d,\
Simply moving UPDATE donation_jailcards onto the same line solved my problem.

I thought I would post this here in case anyone else had a similar problem and in case this is an actual bug, I'm not actually sure. I just found it weird since these queries haven't been changed for 2 years but after updating Zeex's compiler to this experimental version (I do not know the previous) it didn't work properly.

Thanks for such a great compiler, the speed is excellent!

@spacemud
Copy link
Contributor

spacemud commented Nov 4, 2017

This was reported earlier and fixed in a later commit. Just waiting on a release with the fix.

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

Successfully merging this pull request may close these issues.