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

Shorter kernel name for Windows #1701

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Shorter kernel name for Windows #1701

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c92bcb
Shorter kernel name for Windows
zcbenz Dec 14, 2024
0fc68f5
Only hash the clipped part
zcbenz Dec 18, 2024
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
13 changes: 9 additions & 4 deletions mlx/backend/common/compiled_cpu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,20 @@ void* compile(
std::string source_code = source_builder();
std::string kernel_file_name;

// Deal with long kernel names. Maximum length for files on macOS is 255
// characters. Clip file name with a little extra room and append a 16
// character hash.
// Deal with long kernel names. Maximum length for filename on macOS is 255
// characters, and on Windows the maximum length for whole path is 260. Clip
// file name with a little extra room and append a 16 character hash.
#ifdef _WIN32
Copy link
Member

Choose a reason for hiding this comment

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

This totally fine to merge.. but with very short file-names it's not inconceivable to get collisions on file-names (which would be bad). Especially given this. Some ideas to help with that:

I might change the hash to only hash the part of the file name that is clipped to reduce probabilities of collisions. If the prefix is different there won't be a collision anyway. And if it's the same, the suffix must be different and we want to use the suffix for the hash.

We might also want to do some checks on the source.. but that is more involved and we can leave that for future work if it becomes an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will take quite a while before anyone actually starts using the Windows port seriously, I prefer leaving the problem for future.

Copy link
Member

Choose a reason for hiding this comment

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

You can at least make this change or let me edit the PR:

    auto file_id =
        std::hash<std::string>{}(kernel_name.substr(max_file_name_length - 16));

It's simple enough and much safer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please edit the PR for me 🙏

Copy link
Member

Choose a reason for hiding this comment

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

I tried but I don't think I have permission :. Usually it's the default to allow .. but it didn't work in your case. Check out the guide here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is likely because my fork is in an org account, I'll update this PR later.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update!

constexpr int max_file_name_length = 140;
#else
constexpr int max_file_name_length = 245;
#endif
if (kernel_name.size() > max_file_name_length) {
std::ostringstream file_name;
file_name
<< std::string_view(kernel_name).substr(0, max_file_name_length - 16);
auto file_id = std::hash<std::string>{}(kernel_name);
auto file_id =
std::hash<std::string>{}(kernel_name.substr(max_file_name_length - 16));
file_name << "_" << std::hex << std::setw(16) << file_id << std::dec;
kernel_file_name = file_name.str();
} else {
Expand Down