Skip to content

Conversation

mratsim
Copy link

@mratsim mratsim commented May 14, 2018

This brings Nim from

$ time build/bench
331665

real    0m1.222s
user    0m1.193s
sys     0m0.013s

to

$ time build/bench
331665

real    0m0.672s
user    0m0.654s
sys     0m0.012s

on my machine (i5-5257U dual core 2.7Ghz Broadwell)

@ghost
Copy link

ghost commented May 14, 2018

Also it seems that adding flto slowed down Nim (previous results in readme were 0.55 for markAndSweep and 0.95 for default GC)

@frol
Copy link
Owner

frol commented May 14, 2018

@Yardanico That is not LTO, that was a pity bug in the benchmarking code which was fixed in ee670ed

@ghost
Copy link

ghost commented May 14, 2018

@frol ah, ok :)

@frol
Copy link
Owner

frol commented May 14, 2018

Please, fix the conflict. It seems that after you fix it, we will see the diff to be only around {.noinit.}. While I have no idea what that means, it doesn't look to me like something that one would write on a day-to-day basis in production code.

@mratsim
Copy link
Author

mratsim commented May 14, 2018

Basically there is an issue currently in the Nim GC, that triggers it twice when the resulting value is a reference type: one when the Nim sees the result value (cancelled by noInit) and a second time when it's constructed.

I'm fixing the conflict, but seems like I will have to do it from Github interface from my editor it creates new conflict :/.

@mratsim
Copy link
Author

mratsim commented May 14, 2018

It should be good to go now.

@frol
Copy link
Owner

frol commented May 14, 2018

This branch cannot be rebased due to conflicts

@ghost
Copy link

ghost commented May 14, 2018

@frol
image

@mratsim
Copy link
Author

mratsim commented May 14, 2018

Maybe refresh the page?
2018-05-14_21-57-21

@mratsim
Copy link
Author

mratsim commented May 14, 2018

Some screenshots about the difference on the code generated:

Before:

2018-05-14_21-19-47

After:

2018-05-14_21-31-23

genericReset is the code to reset a ref object to pristine state, which is not needed because we always construct the object completely.

@frol
Copy link
Owner

frol commented May 14, 2018

I see you merged master over your branch... Please, consider using git pull --rebase upstream master the next time. It is hard to even review your changes on GitHub as this PR now includes the changes that are already in the master.

@mratsim
Copy link
Author

mratsim commented May 14, 2018

It's strange, I cherry-picked just the Nim changes, sorry :/

2018-05-14_22-04-19

Edit, i can close this PR and redo one from scratch if it's easier.

@frol
Copy link
Owner

frol commented May 14, 2018

I took the new main.nim and compiled it on my Linux machine and it doesn't produce any measurable difference in performance in neither of the configurations. Weird!

@ghost
Copy link

ghost commented May 14, 2018

@frol what about nim devel? :)

@mratsim
Copy link
Author

mratsim commented May 14, 2018

Ah right, the {.noInit.} pragma is ignored on Nim stable due to this regression: nim-lang/Nim#7743

@frol
Copy link
Owner

frol commented May 14, 2018

Well, guys, we are all on stable, I spent the whole day only dealing with stable compilers, and I certainly don't want to maintain all the modifications and tricks with unstable/dev releases. The original idea of the benchmark was to see what we can write in a reasonable time without doing obviously stupid things (e.g. we missed -d:release and used --opt:speed instead in the original version of the benchmark).

@mratsim
Copy link
Author

mratsim commented May 14, 2018

No problem, then I'll reopen a new PR once the new Nim stable hits.

@mratsim mratsim closed this May 14, 2018
@PMunch
Copy link
Contributor

PMunch commented May 15, 2018

Well, guys, we are all on stable, I spent the whole day only dealing with stable compilers, and I certainly don't want to maintain all the modifications and tricks with unstable/dev releases.

Most Nim users tend to use the dev version anyways, there's always neat stuff on those that haven't made it into a release yet. Plus with @mratsims changes and a few changes of my own I was even able to bring the Nim version with markAndSweep down to under C++ raw pointer speed, all while using only 880Kib of memory instead of the 5MiB you have Nim listed at currently..

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.

3 participants