Partially implement cache hash API in zig #4635

leroycep · 2020-03-05T07:26:48Z

Will close #4311

This pull request translates the C++ cache_hash API into zig, under std.cache_hash. Currently it only supports adding files before hitting the cache, doesn't implement the many different cache_* functions, doesn't check files' modification times, and has a memory leak somewhere.

It's a start.

Vexu · 2020-03-05T08:27:05Z

Not sure if this is the way to go, or if I should copy the C API and make a function for every type

The C API should only be copied in stage2.zig.

andrewrk

Nice work so far. I gave a couple suggestions to consider at your leisure

lib/std/cache_hash.zig

andrewrk · 2020-03-06T22:37:02Z

lib/std/cache_hash.zig

+pub const CacheHash = struct {
+    alloc: *Allocator,
+    blake3: Blake3,
+    manifest_dir: []const u8,


I bet this could be improved by using a std.fs.Dir handle rather than a string path name

andrewrk · 2020-03-06T22:38:28Z

lib/std/cache_hash.zig

+const decoder = base64.Base64Decoder.init(base64_alphabet, base64_pad_char);
+const BIN_DIGEST_LEN = 32;
+
+pub const CacheHashFile = struct {


The full path to this struct is std.cache_hash.CacheHashFile so you can remove redundancy by renaming this to simply File. To avoid clashing with fs.File I would suggest removing const File = fs.File; and instead using fs.File when you need that one.

Similar comment for functions which have cache_ in their name.

leroycep · 2020-03-07T23:18:58Z

There are a few pieces missing from the zig std library that are needed to do a full port of the c cache_hash API:

Access to a file identifiers
- FILE_INTERNAL_INFORMATION.IndexNumber on windows
- Stat.ino on linux
An API (probably in std.fs) to open files and put a lock on them

Should I just put all the relevant code in cache_hash, or should I make separate pull requests that add this functionality to the zig std library?

andrewrk · 2020-03-08T01:05:00Z

Should I just put all the relevant code in cache_hash, or should I make separate pull requests that add this functionality to the zig std library?

For these things let's go the std lib route.

LemonBoy · 2020-03-08T23:15:19Z

lib/std/cache_hash.zig

+        debug.assert(self.manifest_file == null);
+
+        const val_type = @TypeOf(val);
+        switch (@typeInfo(val_type)) {


hmm, I wonder if this can be simplified with something like:

self.blake3.update(mem.asBytes(&val));`

Plus some checks for zero-sized types.

This seems like a good idea, I'll try it out. There will also need to be a check for boolean values, because booleans are handled differently (or least were in the C version, but I see no reason to change that).

I think I'll also try to detect fs.File types and have special logic for them.

andrewrk

Is this at a point where it could be merged as is and then improved? Let me know when you're ready for a review.

lib/std/fs.zig

leroycep · 2020-04-14T21:19:06Z

This PR is pretty close, just a few things to add. I think the biggest thing I need to add is logic for adding files after the hash has been generated (cache_add_file in the c code I think). It would also be nice to have a few more tests that make sure that CacheHash doesn't have any false positives.

leroycep · 2020-04-16T02:26:05Z

Okay, I've changed addFile to return the index of the cache_hash.File entry. This means that after checking for a cache hit, you can get the contents of the file that were used in the hashing, like so:

const src_file_idx = try cache_hash.addFile("src/foo.txt");
if (cache_hash.hit()) |hash| {
     // cache hit
} else {
    const src_contents = cache_hash.files.items[src_file_idx].?;
    // compile stuff
}

I'm not sure this is the best API to have. Any feedback welcome!

andrewrk

Thanks for doing this! And thanks for your patience on how long it has taken for me to review it.

I gave some areas to look into improving - let me know when you're ready for round 2.

lib/std/cache_hash.zig

andrewrk · 2020-04-26T18:36:26Z

lib/std/cache_hash.zig

+fn is_problematic_timestamp(file_mtime_ns: i64) bool {
+    const now_ms = time.milliTimestamp();
+    const file_mtime_ms = @divFloor(file_mtime_ns, time.millisecond);
+    return now_ms == file_mtime_ms;
+}


I don't think this has the same behavior as the c++ code:

static bool is_problematic_timestamp(const OsTimeStamp *fs_clock) { OsTimeStamp wall_clock = os_timestamp_calendar(); // First make all the least significant zero bits in the fs_clock, also zero bits in the wall clock. if (fs_clock->nsec == 0) { wall_clock.nsec = 0; if (fs_clock->sec == 0) { wall_clock.sec = 0; } else { wall_clock.sec &= (-1ull) << ctzll(fs_clock->sec); } } else { wall_clock.nsec &= (-1ull) << ctzll(fs_clock->nsec); } return wall_clock.nsec == fs_clock->nsec && wall_clock.sec == fs_clock->sec; }

std.time.milliTimeStamp is not quite what we want here. It looks like std.time.milliTimeStamp is too high level, and std.time does not actually offer what we need: a cross-platform high resolution calendar timestamp. This will require a new function added to std.time. I believe that std.time.milliTimeStamp could then be reworked to call this other function, and do a small amount of logic on top of it.

Basically, move type specific code into their own functions instead of making `add` a giant function responsible for everything.

Now that the memory leak mentioned in ziglang#4656 has been fixed.

It was causing a segfault on `mipsel` architecture, not sure why other architectures weren't affected.

Instead of releasing the manifest file when an error occurs, it is only released when when `CacheHash.release` is called. This maps better to what a zig user expects when they do `defer cache_hash.release()`.

It checks whether the cache will respond correctly to inputs that don't initially depend on filesystem state. In that case, we have to check for the existence of a manifest file, instead of relying on reading the list of entries to tell us if the cache is invalid.

If a user doesn't care that the manifest failed to be written, they can simply ignore it. The program will still work; that particular cache item will simply not be cached.

This makes it possible for the user to retrieve the contents of the file without running into data races.

It occured when the manifest file was manually edited to include an extra file. Now it will simply copy the file name in the manifest file

People using the API as intended would never trigger this assertion anyway, but if someone has a non standard use case, I see no reason to make the program panic.

andrewrk · 2020-05-03T00:43:33Z

@leroycep I'm juggling a few things so I might not be paying close attention on this. Let me know when there is somewhere I can focus my attention to unblock you on this.

andrewrk reviewed Mar 6, 2020

View reviewed changes

leroycep force-pushed the feature-cache-hash-zig branch from 2e5adc1 to e6ed5d9 Compare March 7, 2020 05:46

LemonBoy reviewed Mar 8, 2020

View reviewed changes

leroycep mentioned this pull request Mar 28, 2020

Add lock option to File.OpenFlags and File.CreateFlags #4711

Merged

4 tasks

leroycep force-pushed the feature-cache-hash-zig branch from 88bb407 to 0cdaa72 Compare April 11, 2020 23:44

andrewrk reviewed Apr 14, 2020

View reviewed changes

lib/std/fs.zig Outdated Show resolved Hide resolved

leroycep force-pushed the feature-cache-hash-zig branch from 240a9cb to 69699ba Compare April 16, 2020 02:19

andrewrk requested changes Apr 26, 2020

View reviewed changes

leroycep added 17 commits April 30, 2020 15:42

Partially implement cache hash API in zig

fd6bb4b

Add cache method; add support for caching integers

dd43235

Fix memory leak in cache_hash

d6c7948

Support caching bools; make caching values infallible

130d51e

Add filesystem base64 decoder

6ba700d

Use std.fs.base64_encoder in std.cache_hash

cf88f31

Use fs.File

8c60175

Rename CacheHashFile -> File

0b4067f

Store fs.Dir instead of path to dir

4bd49a5

Rename cache functions to add

0104a2c

Make type specific add functions

a71b60a

Basically, move type specific code into their own functions instead of making `add` a giant function responsible for everything.

Add slice and array support to add method

c3fb2be

Rename cache_file -> addFile

9ee0523

Return base64 digest instead of using an out variable

a67bedf

Make hash digest same size as in the c API

2a04f31

Use readAllAlloc

6435062

Now that the memory leak mentioned in ziglang#4656 has been fixed.

Replace ArrayList in write_manifest with an array

50efd1c

leroycep added 23 commits April 30, 2020 15:42

Check if inode matches inode from manifest

4f085b0

Remove error union from CacheHash.final

5e44336

Remove unnecessary contents field from File

c3b3ee6

It was causing a segfault on `mipsel` architecture, not sure why other architectures weren't affected.

Open file with exclusive lock

f630fef

Check for problematic timestamps

a9b0504

Add documentation to CacheHash API

31625f9

Put base64 alphabet into a named constant

2d9d71f

Add addFilePost and addFilePostFetch functions

b734407

Make CacheHash cleanup consistent (always call release)

7b6016b

Instead of releasing the manifest file when an error occurs, it is only released when when `CacheHash.release` is called. This maps better to what a zig user expects when they do `defer cache_hash.release()`.

Switch to using testing.expect* in tests

5487a09

Add test checking file changes invalidate cache

ce7cec2

Make CacheHash.release return an error

b973846

If a user doesn't care that the manifest failed to be written, they can simply ignore it. The program will still work; that particular cache item will simply not be cached.

Make addFilePost* functions' documentation more clear

8a8d87e

Update code using deprecated ArrayList APIs

3d57536

Return an index from CacheHash.addFile

d207a42

This makes it possible for the user to retrieve the contents of the file without running into data races.

Fix read from null pointer in CacheHash.hit

09e1fa0

It occured when the manifest file was manually edited to include an extra file. Now it will simply copy the file name in the manifest file

Add test case for fix in previous commit

cb07896

Don't use iterate when opening manifest directory

344a32a

Make if statement more idiomatic

9253913

Change null pointer test to addFilePost test

e08edef

Remove non-null assertion in CacheHash.release()

b3d748f

People using the API as intended would never trigger this assertion anyway, but if someone has a non standard use case, I see no reason to make the program panic.

Set manifest's maximum size to Andrew's recommendation

4a5151c

leroycep force-pushed the feature-cache-hash-zig branch from 69699ba to 4a5151c Compare May 1, 2020 02:03

leroycep mentioned this pull request May 1, 2020

Add std.time.nanoTimestamp function #5237

Closed

leroycep added 2 commits May 1, 2020 23:06

Add max_file_size argument

1431e2e

Fix improper initialization of CacheHashFiles

c906e43

andrewrk mentioned this pull request May 24, 2020

zig info command - print lib path, std path and global cache path #5394

Closed

8 tasks

andrewrk closed this in 49f3def May 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partially implement cache hash API in zig #4635

Partially implement cache hash API in zig #4635

leroycep commented Mar 5, 2020 •

edited

Loading

Vexu commented Mar 5, 2020

andrewrk left a comment

andrewrk Mar 6, 2020

andrewrk Mar 6, 2020

leroycep commented Mar 7, 2020

andrewrk commented Mar 8, 2020

LemonBoy Mar 8, 2020

leroycep Mar 24, 2020

andrewrk left a comment

leroycep commented Apr 14, 2020

leroycep commented Apr 16, 2020 •

edited

Loading

andrewrk left a comment

andrewrk Apr 26, 2020 •

edited

Loading

andrewrk commented May 3, 2020

Partially implement cache hash API in zig #4635

Partially implement cache hash API in zig #4635

Conversation

leroycep commented Mar 5, 2020 • edited Loading

Vexu commented Mar 5, 2020

andrewrk left a comment

Choose a reason for hiding this comment

andrewrk Mar 6, 2020

Choose a reason for hiding this comment

andrewrk Mar 6, 2020

Choose a reason for hiding this comment

leroycep commented Mar 7, 2020

andrewrk commented Mar 8, 2020

LemonBoy Mar 8, 2020

Choose a reason for hiding this comment

leroycep Mar 24, 2020

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment

leroycep commented Apr 14, 2020

leroycep commented Apr 16, 2020 • edited Loading

andrewrk left a comment

Choose a reason for hiding this comment

andrewrk Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

andrewrk commented May 3, 2020

leroycep commented Mar 5, 2020 •

edited

Loading

leroycep commented Apr 16, 2020 •

edited

Loading

andrewrk Apr 26, 2020 •

edited

Loading