Skip to content

Massage CityHash to work at comptime - #7331

Closed
SpexGuy wants to merge 1 commit into
ziglang:masterfrom
SpexGuy:fix-comptime-cityhash
Closed

Massage CityHash to work at comptime#7331
SpexGuy wants to merge 1 commit into
ziglang:masterfrom
SpexGuy:fix-comptime-cityhash

Conversation

@SpexGuy

Copy link
Copy Markdown
Contributor

@sinitax ran into issues trying to use CityHash at compile time. This PR modifies the implementation and tests to work at compile time.

@SpexGuySpexGuy added the standard library This issue involves writing Zig code for the standard library. label Dec 7, 2020
@jedisct1

Copy link
Copy Markdown
Contributor

Looking good. Does it have any performance implications?

@SpexGuy

Copy link
Copy Markdown
ContributorAuthor

Performance in debug mode may be impacted by the new offsetPtr function. We could mark offsetPtr and fetch32/fetch64 as inline to mitigate this. But I don't think I've introduced any new bounds checks. Runtime performance in optimized mode should be the same. The comptime execution for the tests is pretty slow (~15 seconds on my computer), so that will affect the total std test time. The change to the tests to write individual bytes instead of type punning may also cause a small increase in test time.

Comment threadlib/std/hash/cityhash.zig Outdated
@memcpy(@ptrCast([*]u8, &v), ptr, 4);
fn fetch32(ptr: [*]const u8, offset: usize) u32 {
// ptr + offset doesn't work at comptime so we need this instead.
const offset_ptr = offsetPtr(ptr, offset)[0..4];

@LemonBoyLemonBoyDec 8, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about

returnmem.readIntNative(u32, offsetPtr(ptr, offset)[0..4]);
// orreturnmem.readIntNative(u32, ptr[offset..offset+4]);

Ditto for fetch64 (and the other two identical fetch helpers? Can you just pull them at top level and delete a copy?)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Huh, I thought these wouldn't work at comptime but apparently they do. Since the next line is byte swap, I can replace the whole function with readIntLittle.

[offset .. offset + 4] works at comptime but at runtime this would produce a slice, so I'm sticking with the other one.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should work at runtime:

returnmem.readIntLittle(u32, ptr[offset..offset+4][0..4]);

(With slices, you can do [offset..][0..4] which is a nice little idiom, but that doesn't work for length-less pointers)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That works but introduces a new but useless runtime check in debug modes (useless because it's only checking that offset + 4 - offset == 4).
ptr[offset .. offset + 4].ptr[0..4] would work, but for consistency I think it's worth sticking with offsetPtr everywhere in this file.

@SpexGuy
SpexGuyforce-pushed the fix-comptime-cityhash branch from 55bc6cb to 1fbd1bfCompareDecember 9, 2020 07:18
@SpexGuy

Copy link
Copy Markdown
ContributorAuthor

Looks like the additional comptime execution for these tests might be putting the test build over the memory limit. That might be a reason to hold off on this until stage 2.

@andrewrk

Copy link
Copy Markdown
Member

Landed in 5cc2e50 but with the comptime execution commented out in the tests. Idea being that we will not have test coverage of this for now, and re-enable them once we ship stage2.

@SpexGuy
SpexGuy deleted the fix-comptime-cityhash branch February 1, 2021 20:39
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

standard libraryThis issue involves writing Zig code for the standard library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@SpexGuy@jedisct1@andrewrk@LemonBoy