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.

Sign up for GitHub

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

Jump to bottom

Restrict COFF to a single thread when symbol count is high #50874

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Restrict COFF to a single thread when symbol count is high #50874

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/aotcompile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,7 @@ static FunctionInfo getFunctionWeight(const Function &F)
}

struct ModuleInfo {
Triple triple;
size_t globals;
size_t funcs;
size_t bbs;
Expand All @@ -662,6 +663,7 @@ struct ModuleInfo {

ModuleInfo compute_module_info(Module &M) {
ModuleInfo info;
info.triple = Triple(M.getTargetTriple());
info.globals = 0;
info.funcs = 0;
info.bbs = 0;
Expand Down Expand Up @@ -1413,6 +1415,13 @@ static unsigned compute_image_thread_count(const ModuleInfo &info) {
LLVM_DEBUG(dbgs() << "32-bit systems are restricted to a single thread\n");
return 1;
#endif
// COFF has limits on external symbols (even hidden) up to 65536. We reserve the last few
// for any of our other symbols that we insert during compilation.
if (info.triple.isOSBinFormatCOFF() && info.globals > 64000) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the usage of magic constants in the code shall be avoided.
This number, 64000, shall be defined as a constant into a specific place in the code, among other constants.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a good reason to define the constant somewhere else, that's more likely to reduce understanding since the constant would be separated from its sole use site. There's also a fairly descriptive comment immediately above the constant describing why it exists, so I don't think the situation would be improved by extracting it into another variable and naming it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the details of aotcompile.hpp and aotcompile.cpp, but imagine you a situation where you have 20 such magic constants in the code and at some point in time a newer colleague will want to change one such constant. He will have to search the code using the magic constant (he does not know the exact value) and good luck finding that piece of code. In general is best to have these static const variables grouped for instance in a class Params in the header or at the beginning of the cpp file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, if someone is modifying this piece of code, which is the only one that depends on this constant, it makes more sense for the value to be right here. Specially because it has the comment right above it.
This isn't a parameter to be played with.

LLVM_DEBUG(dbgs() << "COFF is restricted to a single thread for large images\n");
return 1;
}

// This is not overridable because empty modules do occasionally appear, but they'll be very small and thus exit early to
// known easy behavior. Plus they really don't warrant multiple threads
if (info.weight < 1000) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yet another magic constant (not because of this PR)

and line 1434 contains another one:
auto max_threads = info.globals / 100

Expand Down