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

Update Time.cs #183

Merged
merged 1 commit into from
May 20, 2020
Merged

Update Time.cs #183

merged 1 commit into from
May 20, 2020

Conversation

DemoXinMC
Copy link
Contributor

Added conversions to and from .NET's TimeSpan struct
Significantly reduced P/Invoke calls, which now only happen during construction and conversion

This should yield performance gains in any place where Time is being used. Additionally, this is a bit of futureproofing for SFML3, as Time is likely to be removed, and the TimeSpan struct has existed in .NET since Framework 1.0

@eXpl0it3r eXpl0it3r added the Feature New functionality or extending of an existing one label Feb 17, 2020
src/SFML.System/Time.cs Outdated Show resolved Hide resolved
src/SFML.System/Time.cs Outdated Show resolved Hide resolved
src/SFML.System/Time.cs Show resolved Hide resolved
@LaurentGomila
Copy link
Member

This should yield performance gains in any place where Time is being used

Is this something that you have measured? Rewriting parts of SFML instead of direct calls, has a maintenance cost and should only be done if really necessary.

@DemoXinMC
Copy link
Contributor Author

I specifically discovered the performance hit when using SFML.System.Time as part of my animating sprite setup and discovered it to be the single largest bottle neck I have. P/Invoke calls add up very quickly, and considering that this is a very minor change for a class that isn't very volatile, I think this is a worthwhile bit of code copying.

TimeSpan exists within .NET, and serves an identical purpose, but this is the simplest way to allow the use of TimeSpan without rewriting existing signatures to cover a custom marshaler.

When SFML 3 becomes available, I plan to remove the use of sfTime in favor of TimeSpan and a custom marshaler.

@eXpl0it3r
Copy link
Member

While I see the cost for re-implementing code, I do think this one is justified, given that it's quite common to call the conversion functions a lot and you wouldn't expect P/Invoke calls for such a simple operation.

@DemoXinMC
Copy link
Contributor Author

Any further thoughts on this?

@LaurentGomila
Copy link
Member

timeSpan.Ticks / (TimeSpan.TicksPerMillisecond / 1000)

I know it's not the case here, but if TimeSpan.TicksPerMillisecond was not an exact multiple of 1000, this would be incorrect. A 100% safe (future-proof) version would simply be:

timeSpan.Ticks * 1000 / TimeSpan.TicksPerMillisecond

@DemoXinMC
Copy link
Contributor Author

DemoXinMC commented Mar 22, 2020

Those 2 formulas yield the same result even with non-multiples of 1000

timeSpan.Ticks / (TimeSpan.TicksPerMillisecond / 1000)
20 / (13340 / 1000)
20 / 13.34
1.499250374812594

timeSpan.Ticks * 1000 / TimeSpan.TicksPerMillisecond
20 * 1000 / 13340
20000 / 13340
1.499250374812594

@LaurentGomila
Copy link
Member

In floating-point arithmetic yes, not with integer division -- unless C# has different rules than C++ for that.

Added conversions to and from .NET's TimeSpan struct
Significantly reduced P/Invoke calls, which now only happen during construction and conversion
@DemoXinMC
Copy link
Contributor Author

Fair enough. Formula changed and PR updated.

@eXpl0it3r eXpl0it3r merged commit 620699d into SFML:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality or extending of an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants