WaveSabre icon indicating copy to clipboard operation
WaveSabre copied to clipboard

Use std::atomic instead of the windows-only primitives for atomic operations in the song renderer

Open PoroCYon opened this issue 5 years ago • 6 comments

I've started splitting up my patches into usable chunks, this is the first one.

Someone will have to test whether this compiles to the atomic instructions on x86 w/ MSVC, but I'm pretty sure it does do that.

PoroCYon avatar Sep 09 '20 15:09 PoroCYon

~~I have no clue why it doesn't compile, and emoon's new[] implementation doesn't seem to help either.~~ I should try doing this stuff when I'm actually awake.

(I'll squash the commits once it's working.)

PoroCYon avatar Sep 09 '20 16:09 PoroCYon

Minus those one (or two) nit(s) above, this looks pretty good to me.

kusma avatar Sep 14 '20 08:09 kusma

This one needs to be size-tested and verified working especially with squishy, which also doesn't support TLS (most 64k packers don't actually). I have a task in squishy to identify TLS usage and error in that case (a user has accidentally tripped on this and provided a test case) and I'd like to do that first before double-checking that this is OK. (if this becomes a bottleneck, this can be checked for manually btw).

yupferris avatar Sep 14 '20 10:09 yupferris

Ah, I see now that we've indeed split the impl between windows and other platforms - that makes me more confident about size and checking for TLS with squishy is actually a non-issue in that case. Still, I'd like to see a size comparison (though I expect this will not really be affected, as the code will be monomorphized and likely inlined).

yupferris avatar Sep 14 '20 10:09 yupferris

@yupferris: Yeah... We should probably make the CI report the size of the stand-alone player (or TestPlayer) or something, so we always know what's going on there. It's tempting to try to turn this into some sort of delta and make it fail if it grows etc, but tracking stats across CI runs is much more difficult than just reporting numbers in the log. So maybe just printing the size at the end is enough?

kusma avatar Sep 14 '20 11:09 kusma

For many changes we should still ideally run the thing ofc, but at least for a first approximation (and something we can link to for actual results/bookkeeping) it's a good idea to pack and report in CI. I don't think we need (or necessarily want) to do more than that.

yupferris avatar Sep 14 '20 12:09 yupferris