fix(cache): handle missing cache hits when chaining two run steps #25788
+285,065
−55,886
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bfredl
force-pushed
the
cache4
branch
2 times, most recently
from
8b75cfe to
a9e757d
Compare
This API is a bit too opinionated for the Zig standard library. Applications should contain this logic instead.
These symbols are already provided by compiler_rt
These symbols are already provided by compiler_rt
… packets This addresses the regression specific to GitHub's chunked transfer encoding for larger repositories while leaving existing functionality intact.
Reject low-order points by checking projective coordinates directly instead of using affine coordinates. Equivalent, but saves CPU cycles (~254 field multiplications total before, 3 field multiplications after).
Fixes a issue in tryFindProgram where it would fail if the PATH environment variable contained relative paths, due to its incorrect assumption that the full_path argument is always absolute path. Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30619 Co-authored-by: hixuyuming <[email protected]> Co-committed-by: hixuyuming <[email protected]>
See https://codeberg.org/ziglang/zig/issues/30637 for details. Works around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86673 Co-authored-by: Laurin-Luis Lehning <[email protected]> Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30645 Reviewed-by: Andrew Kelley <[email protected]> Co-authored-by: e820 <[email protected]> Co-committed-by: e820 <[email protected]>
…0650) from jedisct1/zig:ed25519rej into master Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30650
When linking libc, it should be the libc that manages the heap. The main Wasm memory might have been configured as non-growable, which makes `WasmAllocator` a poor default and causes the common `DebugAllocator` use case fail with OOM errors unless the user uses `std_options` to override the default page allocator. Additionally, on Emscripten, growing Wasm memory without notifying the JS glue code will cause array buffers to get detached and lead to spurious crashes.
…Wasm + libc" This reverts commit c9fa8e4. This commit was failing CI checks. This failure was unfortunately not noticed before merge, due in part to the build runner bug fixed in the last commit.
The goal of this internal refactor is to fix some bugs in cancelation and allow group tasks to clean up their own resources eagerly. The latter will become a guarantee of the `std.Io` interface, which is important so that groups can be used to "detach" tasks. This commit changes the API which POSIX system calls use internally (the functions formerly called `beginSyscall` etc), but does not update the usage sites yet.
The most interesting thing here is the replacement of the pthread futex implementation with an implementation based on thread park/unpark APIs. Thread parking tends to be the primitive provided by systems which do not have a futex primitive, such as NetBSD, so this implementation is far more efficient than the pthread one. It is also useful on Windows, where `RtlWaitOnAddress` is itself a userland implementation based on thread park/unpark; we can implement it ourselves including support for features which Windows' implementation lacks, such as cancelation and waking a number of waiters with 1<n<infinity. Compared to the pthread implementation, this thread-parking-based one also supports full robust cancelation. Thread parking also turns out to be useful for implementing `sleep`, so is now used for that on Windows and NetBSD. This commit also introduces proper cancelation support for most Windows operations. The most notable omission right now is DNS lookups through `GetAddrInfoEx`, just because they're a little more work due to having a unique cancelation mechanism---but the machinery is all there, so I'll finish gluing it together soon. As of this commit, there are very few parts of `Io.Threaded` which do not support full robust cancelation. The only ones which actually really matter (because they could block for a prolonged period of time) are DNS lookups on Windows (as discussed above) and futex waits on WASM.
As of this branch, the performance impact of robust cancelation is now negligible (and in fact entirely unmeasurable in almost all cases), so there is no good reason to not enable it in all cases. The performance issues before were primarily down to a typo in the robust cancelation logic which resulted in every canceled syscall potentially being sent hundreds of signals in quick succession, because the delay between signals started out at 1ns instead of 1us!
This commit includes some API changes which I agreed with Andrew as a follow-up to the recent `Io.Group` changes: * `Io.Group.await` *does* propagate cancelation to group tasks; it then waits for them to complete, and *also* returns `error.Canceled`. The assertion that group tasks handle `error.Canceled` "correctly" means this behavior is loosely analagous to how awaiting a future works. The important thing is that the semantics of `Group.await` and `Future.await` are similar, and `error.Canceled` will always be visible to the caller (assuming correct API usage). * `Io.Group.awaitUncancelable` is removed. * `Future.await` calls `recancel` only if the "child" task (the future being awaited) did not acknowledge cancelation. If it did, then it is assumed that the future will propagate `error.Canceled` through `await` as needed.
This was missed when updating to the new group cancelation API, and caused illegal behavior in many cases (the condition was simply that a DNS query returned a second result before a connection was successfully established).
Previously, 64-bit '<<|' operations were emitting 64-bit shifts with one 64-bit operand and one 32-bit operand, which is illegal. Instead, as in the lowering for regular shifts, we need to cast the RHS in this case.
Change the log implementation to prepend the current target and update to all logs which happen during an update. Makes progress on ziglang#22510, but does not fully resolve it.
…es, and better Windows and NetBSD support' (#30634) from std.Io.Threaded-groups-2 into master Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30634 Reviewed-by: Andrew Kelley <[email protected]> Resolves: https://codeberg.org/ziglang/zig/issues/30049
Change-Id: Iba9c4bf2cfa4ff1b82dd5f0828c57711f238f1bf
…ckaddr (#30722) Resolves ziglang/zig#30672 - UB caused by `std.Io.Threaded.netLookupFallible` incorrectly initializing `PosixAddress`/`WsaAddress` from `*sockaddr`. Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30722 Co-authored-by: moriazoso <[email protected]> Co-committed-by: moriazoso <[email protected]>
clock_nanosleep is specified by POSIX but not implemented on these hereby shamed operating systems: * macOS * OpenBSD (which defines TIMER_ABSTIME for some reason...?)
…zig:openbsd-ci into master Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30733
54a8496 did this for std.os.linux.SIG but I neglected to also do it for std.c.SIG
…y' (#30746) from clock_nanosleep into master Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30746
This file has changed a lot since the previous release, and I resisted the urge to do this until the conflicts would be minimized.
* also remove musl implementation
ABI detection previously did not take into account the non-standard directory structure of Android. This has been fixed. The API level is detected by running `getprop ro.build.version.sdk`, since we don't want to depend on bionic, and reading system properties ourselves is not trivially possible.
The previous logic was made really messy by the fact that upon entry to the step eval worker, the step may not be ready to run, we may be racing with other workers doing the same check, and we had already acquired our RSS requirement even though we might not run. It also required iterating all dependencies each time we were called to check whether we were even ready to run yet. A much better strategy is for each step to have an atomic counter representing how many of its dependencies are yet to complete. When a step completes (successfully or otherwise), it decrements this value for all of its dependants, and if it drops any to 0, it schedules that step to run. This means each step is scheduled exactly once, and only when all of its dependencies have finished, reducing redundant checks and hence contention. If the step being scheduled needs to claim RSS which isn't available, then it is instead added to `memory_blocked_steps`, which is iterated by the step worker after a step with an RSS claim finishes. This logic is more concise than before, simpler to understand, generally more efficient, and fixes a bug in the RSS tracking. Also, as a nice side effect, it should also play a little bit nicer with `Io.Threaded`'s scheduling strategy, because we no longer spawn extremely short-lived tasks all the time as we previously did. Resolves: https://codeberg.org/ziglang/zig/issues/30742
The implementation of HostName.validate was too generous. It considered strings like ".example.com", "exa..mple.com", and "-example.com" to be valid hostnames, which is incorrect according to RFC 1123 (the currently accepted standard). Reviewed-on: ziglang#25710
…AAAA records closes ziglang#25948
Follow-up to https://codeberg.org/ziglang/zig/pulls/30746. The TIMER_ABSTIME value was adjusted to match other systems in SerenityOS/serenity#26543.
…ther than entropy' (#30736) from jedisct1/zig:edsigned into master Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30736 Reviewed-by: Andrew Kelley <[email protected]>
… of direct entropy' (#30738) from jedisct1/zig:scryptfixes into master Reviewed-on: https://codeberg.org/ziglang/zig/pulls/30738 Reviewed-by: Andrew Kelley <[email protected]>
fixes ziglang#19817 This improves the efficiency of the cache when chaining muliple commands like const step1 = b.addRunArtifact(tool_fast); step1.addFileArg(b.path("src/input.c")); const output1 = step1.addOutputFileArg("output1.h"); const step2 = b.addRunArtifact(tool_slow); step2.addFileArg(output1); const chained_output = step2.addOutputFileArg("output2.h"); assume that step2 takes much long time than step1 if we make a change to "src/input.c" which produces an identical "output1.h" as a previous input, one would expect step2 not to rerun as the cached output2.h only depends on the content of output1.h However, this does not work yet as the hash of src/input.c leaks into the file name of the cached output1.h, which the second run step interprets as a different cache key. Not using the ".zig-build/o/{HASH}" part of the file name in the hash key fixes this.
Member
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #19817
This is the same as #19974 but un-bittrotted
This improves the efficiency of the cache when chaining multiple commands like
assume that step2 takes much long time than step1
if we make a change to "src/input.c" which produces an identical "output1.h" as a previous input, one would expect step2 not to rerun as the cached output2.h only depends on the content of output1.h
However, this does not work yet as the hash of src/input.c leaks into the file name of the cached output1.h, which the second run step interprets as a different cache key. Not using the ".zig-build/o/{HASH}" part of the file name in the hash key fixes this.
https://github.com/bfredl/zig-run4run is updated for zig 0.16 as a demonstration. e.g. in
src/foo.cchangingimplementation_of_foo()tonew_implementation_of_foo()should only rerun the first step, not the second.