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

Correct check for Windows compilation #931

Merged
merged 1 commit into from
May 7, 2024

Conversation

pps83
Copy link
Contributor

@pps83 pps83 commented Apr 1, 2024

_WIN32 is the proper check for compilation for Windows. WIN32 is defined when Windows.h is included or could be defined by some projects

`_WIN32` is the proper check for compilation for Windows. `WIN32` is defined when `Windows.h` is included or could be defined by some projects
@pps83
Copy link
Contributor Author

pps83 commented Apr 1, 2024

@Cyan4973 Cyan4973 self-assigned this Apr 1, 2024
@pps83
Copy link
Contributor Author

pps83 commented Apr 1, 2024

similar fix should be merged into zstd (plus all defines for WIN32 from projects should be removed, but that's irrelevant).
as a side note, what's the reason xxhash in lz4 is stuck using version form 6 years ago, why isn't it updated?

@Cyan4973
Copy link
Owner

Cyan4973 commented Apr 1, 2024

what's the reason xxhash in lz4 is stuck using version form 6 years ago, why isn't it updated?

liblz4 proper only requires XXH32, which hasn't evolved.

We can still update libxxhash though. Just not critical.

@pps83
Copy link
Contributor Author

pps83 commented Apr 1, 2024

liblz4 proper only requires XXH32, which hasn't evolved.

zstd stays more or less up to date with xxhash because it uses XXH64, is that correct? From xxhash the fastest one is XXH3, what percentage roughly is hash compute of zstd? I'm mainly interested in decompression speed, so I'm ok to break zstd compat as it's used only in my own code and to modify it to use XXH3 or even avoid hashing completely. Is there any issues to update zstd hashing to XXH3 or to a null hash?

@Cyan4973
Copy link
Owner

Cyan4973 commented Apr 1, 2024

You can check the decompression speed with and without the checksum, on both the library and the CLI sides.
This will tell you how much the checksum costs in your case.

The result is very dependent on the data and the hardware system, I would expect something in the 10% range but variance is high.

@Cyan4973 Cyan4973 merged commit 5930eb4 into Cyan4973:dev May 7, 2024
63 checks passed
@pps83 pps83 deleted the dev-win32-sheck branch May 9, 2024 10:23
petk added a commit to petk/php-src that referenced this pull request Jun 4, 2024
petk added a commit to php/php-src that referenced this pull request Jun 10, 2024
* Replace WIN32 conditions with _WIN32 or PHP_WIN32

WIN32 is defined by the SDK and not defined all the time on Windows by
compilers or the environment. _WIN32 is defined as 1 when the
compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise,
undefined.

This syncs these usages one step further.

Upstream libgd has replaced WIN32 with _WIN32 via
libgd/libgd@c60d9fe

PHP_WIN32 is added to ext/sockets/sockets.stub.php as done in other
*.stub.php files at this point.

* Use PHP_WIN32 in ext/random

* Use PHP_WIN32 in ext/sockets

* Use _WIN32 in xxhash.h as done upstream

See Cyan4973/xxHash#931

* Update end comment with PHP_WIN32
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.

2 participants