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

[jsonifier, discordcoreapi] Fix build hangs and warnings. #42355

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Nov 25, 2024

2 potential causes:

  • At least in my testing, build requires >9GB per thread of memory, which might have been 'hanging' by causing the OOM killer to kill the agent.
  • There are 'sub' CMake builds which write to the source tree and stomp on each other.

Related: RealTimeChris/Jsonifier#35
Related: #40229

2 potential causes:
* At least in my testing, build requires >9GB per core of memory, which might have been 'hanging' by causing the OOM killer to kill the agent.
* There are 'sub' CMake builds which write to the source tree and stomp on each other.

Related: RealTimeChris/Jsonifier#35
@BillyONeal BillyONeal added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Nov 25, 2024
@BillyONeal
Copy link
Member Author

I'm hoping this also fixes the build being very very slow on osx:

image

since this is forcing those machines into heavy swap use.

@BillyONeal
Copy link
Member Author

/cc @RealTimeChris

@JonLiu1993 JonLiu1993 self-assigned this Nov 26, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Nov 26, 2024

/cc @RealTimeChris

TBH discordcoreapi is fairly strange CMake code.
The port should be disabled in CI, if not delisted, until revised.

As an addtional note, the package optimizes for the capabilities of the physical host CPU. It is unable to cross-build.

Taking a second look, this trouble starts at jsonifier.

(I do acknowledge that the source code offers excellent optimizations. I'm unhappy with CMake code and the ports.)

@RealTimeChris
Copy link
Contributor

RealTimeChris commented Nov 26, 2024

The build taking a long time/extreme RAM consumption should be fixed by the new tuple implementation I've added to Jsonifier since its last release. Let me push the changes to main.

@RealTimeChris
Copy link
Contributor

RealTimeChris commented Nov 26, 2024

/cc @RealTimeChris

TBH discordcoreapi is fairly strange CMake code. The port should be disabled in CI, if not delisted, until revised.

As an addtional note, the package optimizes for the capabilities of the physical host CPU. It is unable to cross-build.

Taking a second look, this trouble starts at jsonifier.

(I do acknowledge that the source code offers excellent optimizations. I'm unhappy with CMake code and the ports.)

You can cross-compile, you just need to set JSONIFIER_CPU_INSTRUCTIONS flag to correctly align with whichever target CPU architecture you're compiling for.
https://github.com/RealTimeChris/Jsonifier/blob/main/Documentation/CPU_Architecture_Selection.md

@BillyONeal
Copy link
Member Author

Looks like this fixed the warnings on Windows, taking forever on macOS, and the OOM killer on Linux:

Elapsed time to handle discordcoreapi:x64-osx: 1 min
Elapsed time to handle discordcoreapi:x64-linux: 35 s

@BillyONeal
Copy link
Member Author

TBH discordcoreapi is fairly strange CMake code.
The port should be disabled in CI, if not delisted, until revised.

I agree that, for example, invoking a sub complete CMake build is a little unorthodox, but unless it is causing problems for other ports I don't think it should be deindexed for that.

As an addtional note, the package optimizes for the capabilities of the physical host CPU. It is unable to cross-build.

That's true, but it's hardly the only port to do that. I don't think we have settled on a great answer to this situation: It's true that changing stuff based on the host CPU is bad for binary caching, but the alternative of force disabling all such optimizations makes ports that care about doing this useless for their customers much of the time, so we haven't tried to ban it despite the potential issues.

The build taking a long time/extreme RAM consumption should be fixed by the new tuple implementation I've added to Jsonifier since its last release. Let me push the changes to main.

Awesome, thanks!

@BillyONeal BillyONeal changed the title [jsonifier, discordcoreapi] Attempt to fix build hangs and warnings. [jsonifier, discordcoreapi] Fix build hangs and warnings. Nov 26, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Nov 26, 2024

You can cross-compile, you just need to set JSONIFIER_CPU_INSTRUCTIONS flag to correctly align with whichever target CPU architecture you're compiling for.

I started to study this already.

Jsonifier comes as header-only. It shouldn't need to commit to a particular CPU, but it burns the host CPU type into a header. Even worse, it forces its compile flags on consumers. -march=native (GNU, CLANG) as well as /fp:fast (MSVC).

@RealTimeChris
Copy link
Contributor

RealTimeChris commented Nov 26, 2024

You can cross-compile, you just need to set JSONIFIER_CPU_INSTRUCTIONS flag to correctly align with whichever target CPU architecture you're compiling for.

I started to study this already.

Jsonifier comes as header-only. It shouldn't need to commit to a particular CPU, but it burns the host CPU type into a header. Even worse, it forces its compile flags on consumers. -march=native (GNU, CLANG) as well as /fp:fast (MSVC).

My mistake the -march=native should have never been in there and I've removed the /fp:fast from msvc. Also I will look into trying runtime methods for detecting the CPU architecture, as long as it doesn't ruin performance.

@BillyONeal BillyONeal merged commit efb2be8 into microsoft:master Nov 27, 2024
15 of 17 checks passed
@BillyONeal BillyONeal deleted the discordcoreapi branch November 27, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants