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

Crystal structs instead of classes #1

Closed
jblindsay opened this Issue Aug 18, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@jblindsay
Copy link

jblindsay commented Aug 18, 2016

Very interesting project. I would suggest changing all of the 'class' declarations in the Crystal code to 'struct' instead. This is idiomatic Crystal (https://crystal-lang.org/docs/guides/performance.html) and will increase the performance by several factors. In fact, on my machine, this (i.e. just changing the class to struct and changing nothing else) places the Crystal code that you have written at a speed very similar to Nim and C.

@niofis

This comment has been minimized.

Copy link
Owner

niofis commented Aug 18, 2016

Thanks for your suggestion, I was wondering exactly the same when writing the implementation, but somewhere in the docs said that structs pass as value instead of references, and assumed the speed would be impacted. I'll make the change and upload the results soon.

Thank you again!

@jblindsay

This comment has been minimized.

Copy link

jblindsay commented Aug 18, 2016

Glad to be of help. I think you'll find that that one small change has an enormous impact on the performance. It's because structs are allocated on the stack rather than the heap.

@niofis

This comment has been minimized.

Copy link
Owner

niofis commented Aug 18, 2016

I already made and committed the changes, as you pointed out, the results
are way better. Thanks!

On Wed, Aug 17, 2016 at 5:29 PM, John Lindsay notifications@github.com
wrote:

Glad to be of help. I think you'll find that that one small change has an
enormous impact on the performance. It's because structs are allocated on
the stack rather than the heap.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABHc0o_OJ9BCe07-PJQbt8Fb_NJeLyDUks5qg6dPgaJpZM4JnAbq
.

@asterite

This comment has been minimized.

Copy link

asterite commented Aug 18, 2016

Also, don't use $globals, use CONSTANTS. In your code the globals you have, like zero, width and height, are all really constants. This should allow the compiler to do some more optimizations (though I doubt they will be significant, but using globals is not idiomatic and in fact globals will soon be removed from the language)

@asterite

This comment has been minimized.

Copy link

asterite commented Aug 18, 2016

Actually, I see you use globals iside the main ray tracing loop, so I think some improvement will be visible

@niofis

This comment has been minimized.

Copy link
Owner

niofis commented Aug 18, 2016

@asterite Applied the changes you suggested by using constants instead of globals, but performance was not affected. I still kept the changes because of your comment that globals might soon be removed from the language.

Results from three consecutive runs:

$ time ./crrb

real    2m1.602s
user    2m1.008s
sys 0m0.144s
$ time ./crrb

real    2m1.762s
user    2m1.136s
sys  0m0.180s
$ time ./crrb

real    2m2.274s
user    2m1.568s
sys  0m0.132s

Thanks for your input.

@asterite

This comment has been minimized.

Copy link

asterite commented Aug 18, 2016

Thanks! I tried it too and didn't notice a difference, reading those globals is probably insignificant relative to the other code (or maybe LLVM is very good at optimizing this)

@niofis niofis closed this Aug 19, 2016

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