Delay finding and associating a build file with a (handle) uri #1647

llogick · 2023-12-03T01:25:56Z

Current logic requires the modules available in a build.zig file to be known in order to correctly associate the file with an uri.

See #1629

Take N2

codecov · 2023-12-03T01:27:38Z

Codecov Report

Attention: 95 lines in your changes are missing coverage. Please review.

Comparison is base (5c0bebe) 76.12% compared to head (340ac48) 75.89%.

Files	Patch %	Lines
src/DocumentStore.zig	38.46%	72 Missing ⚠️
src/analysis.zig	12.50%	7 Missing ⚠️
src/features/completions.zig	22.22%	7 Missing ⚠️
src/Server.zig	42.85%	4 Missing ⚠️
src/ComptimeInterpreter.zig	0.00%	2 Missing ⚠️
src/features/goto.zig	0.00%	2 Missing ⚠️
src/features/references.zig	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1647      +/-   ##
==========================================
- Coverage   76.12%   75.89%   -0.24%     
==========================================
  Files          34       34              
  Lines       10007    10022      +15     
==========================================
- Hits         7618     7606      -12     
- Misses       2389     2416      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Techatrix · 2023-12-05T18:50:05Z

src/DocumentStore.zig

@@ -484,7 +484,9 @@ runtime_zig_version: *const ?ZigVersionWrapper,
 lock: std.Thread.RwLock = .{},
 handles: std.StringArrayHashMapUnmanaged(*Handle) = .{},
 build_files: std.StringArrayHashMapUnmanaged(*BuildFile) = .{},
+build_files_lock: std.Thread.RwLock = .{},


Why is a new RwLock for build files needed?

Techatrix · 2023-12-05T18:57:55Z

src/Server.zig

    load_build_configuration: DocumentStore.Uri,
    run_build_on_save,

    fn deinit(self: Job, allocator: std.mem.Allocator) void {
        switch (self) {
            .incoming_message => |parsed_message| parsed_message.deinit(),
            .generate_diagnostics => |uri| allocator.free(uri),
+            .find_build_file_for_uri => |uri| allocator.free(uri),


these switch cases here can be combined.

Techatrix · 2023-12-05T18:59:42Z

src/DocumentStore.zig

+    if (std.process.can_spawn and !self.bare_instance_no_server) {
+        const Server = @import("Server.zig");
+        const server = @fieldParentPtr(Server, "document_store", self);
+
+        server.job_queue_lock.lock();
+        defer server.job_queue_lock.unlock();
+
+        try server.job_queue.ensureUnusedCapacity(1);
+        server.job_queue.writeItemAssumeCapacity(.{
+            .find_build_file_for_uri = try server.allocator.dupe(u8, uri),
+        });
+    }


I believe we can avoid this dependency on Server and the new bare_instance_no_server by making the DocumentStore take in a ?*const std.Thread.Pool at init time.

Techatrix · 2023-12-05T19:03:56Z

src/DocumentStore.zig

+}
+
+pub fn findBuildFileForUri(self: *DocumentStore, uri: Uri) error{OutOfMemory}!void {
+    const handle = self.getHandle(uri) orelse return;
    if (isBuildFile(handle.uri) and !isInStd(handle.uri)) {
        _ = self.getOrLoadBuildFile(handle.uri);


This is unrelated to finding the associated_build_file so why does this happen here?

Techatrix · 2023-12-05T19:06:41Z

src/DocumentStore.zig

+    if (std.process.can_spawn and !self.bare_instance_no_server) {
+        const Server = @import("Server.zig");
+        const server = @fieldParentPtr(Server, "document_store", self);
+
+        server.job_queue_lock.lock();
+        defer server.job_queue_lock.unlock();
+
+        try server.job_queue.ensureUnusedCapacity(1);
+        server.job_queue.writeItemAssumeCapacity(.{
+            .find_build_file_for_uri = try server.allocator.dupe(u8, uri),
+        });
+    }


You have also delayed the collection imports and cimports which can cause wrong results until findBuildFileForUri is done.

The code that prints the Opened document {s} with build file {s} message should also be removed because there is no guarantee that at that point in time, the associated build file has been found.

Techatrix · 2023-12-05T19:08:31Z

src/DocumentStore.zig

+    return handle;
+}
+
+pub fn findBuildFileForUri(self: *DocumentStore, uri: Uri) error{OutOfMemory}!void {


This function potentially writes to associated_build_file, import_uris and cimports of Handle while other threads may read from them -> Data Race

Techatrix · 2023-12-05T19:44:20Z

I just realized that you had converted your PR into a Draft, sorry for doing the review too early.

llogick · 2023-12-05T21:35:33Z

I just realized that you had converted your PR into a Draft, sorry for doing the review too early.

It's actually appreciated — thank you.

I'm scrapping (the approach on) this one and could use ideas on how to solve it.

Haven't tested but I thought about refactoring out the find build file logic into a fn (similar to the one in this PR) and if there are no matches here

zls/src/DocumentStore.zig

Lines 1420 to 1424 in b328500

    
           for (build_config.packages) |pkg| { 
        
               if (std.mem.eql(u8, import_str, pkg.name)) { 
        
                   return try URI.fromPath(allocator, pkg.path); 
        
               } 
        
           }

call it again? Con: extra file system operations

or

Keep a handle.potential_build_zig_files: []Uri and check those.

Techatrix · 2023-12-05T22:24:18Z

Having one function that does the entire "search for build file" job would be a great improvement.

This is what I'm thinking of to get the issue resolved:
https://github.com/zigtools/zls/blob/b3285004f11b0a5f632a778685dcc8c1fcf50cf9/src/DocumentStore.zig#L1034C85-L1034C85
collectBuildConfigPackageUris is being called while finding the associated_build_file of a document. if we ever encountered it returning false (which indicates that build config was not available) and we failed to find the associated build file at the end, then we know that we may able to find a build file once the build config(s) have been resolved.
So we just need to somehow await for the build config(s) to become available and then redo the build file search. If only we had async await...

This can be viewed a separate issue but its kind of related:
There are also some problems with how we store resolved import URIs for each handle because they depend on zig_lib_path, associated_build_file and their build config. If any of these change, we need to invalidate them. This issue is often unnoticed because editing a document will refresh the import URIs so they will quickly get corrected.

Techatrix · 2023-12-10T21:58:58Z

src/DocumentStore.zig

@@ -1063,7 +1101,7 @@ fn uriInImports(
    };
    gop.key_ptr.* = handle.uri;

-    if (handle.associated_build_file) |associated_build_file_uri| {
+    if (handle.associated_build_file orelse try self.doubleCheckPotentialBuildFileUri(handle)) |associated_build_file_uri| {


reading from handle.associated_build_file should be protected with a lock because other threads may write into it by calling doubleCheckPotentialBuildFileUri on the same handle.

I would suggest to replace handle.associated_build_file orelse try self.doubleCheckPotentialBuildFileUri(handle) with something like try handle.getAssociatedBuildFile(store) and hide the associated_build_file field by moving it into the impl struct of Handle.

Within that fn, is a return self.associated_build_file; guaranteed to make a copy or should the lock be held outside?

Within the function, you are holding a lock so the read from self.associated_build_file is protected but outside the function, you are not holding a lock which makes accessing self.associated_build_file a potential data race.
In short, always hold the lock of the handle when accessing self.associated_build_file.

Does that make it more clear what the problem is?

I get the data race, my question is if the handle.getAssociatedBuildFile(store) is

// lock defer unlock here return self.associated_build_file;

is it guaranteed to make a copy which can be safely read outside the fn or should it be similar toBuildFile.tryLockConfig

Techatrix · 2023-12-10T21:59:20Z

src/DocumentStore.zig

+    switch (handle.potential_build_file_uris.items.len) {
+        0 => {},
+        else => |count| blk: {


This nesting can be removed.

Techatrix · 2023-12-10T22:01:26Z

src/DocumentStore.zig

+    pub fn hasConfig(self: *BuildFile) bool {
+        self.impl.mutex.lock();
+        defer self.impl.mutex.unlock();
+        return if (self.impl.config) |_| true else false;


Suggested change

return if (self.impl.config) |_| true else false;

return self.impl.config != null;

Techatrix · 2023-12-10T22:06:30Z

src/DocumentStore.zig

+                    handle.associated_build_file = handle.potential_build_file_uris.items[index];
+                    break :ok true;
+                }
+            } else unreachable;


Why is the else unreachable?

The for is guaranteed at least one entry/iteration.

How is the number of iterations related to whether or the not the else path is reached?

for (handle.potential_build_file_uris.items[1..], 1..) |build_file_uri, index| { const build_file = self.getOrLoadBuildFile(build_file_uri) orelse break :ok false; if (!build_file.hasConfig()) break :ok false; if (try self.uriAssociatedWithBuild(build_file, handle.uri)) { handle.associated_build_file = handle.potential_build_file_uris.items[index]; break :ok true; } // if this code path here is reached on the last iteration the you hit unreachable. // I don't see why it shouldn't be possible to reach on the last iteration. // The handle may not be associated with any of the potential build files. } else unreachable

TIL; I thought it'd be only if the condition failed.

Techatrix · 2023-12-10T22:15:07Z

src/DocumentStore.zig

+            _ = self.getOrLoadBuildFile(handle.potential_build_file_uris.items[0]) orelse break :blk;
+            handle.associated_build_file = handle.potential_build_file_uris.items[0];
+            if (count == 1) break :blk;
+            const ok: bool = ok: for (handle.potential_build_file_uris.items[1..], 1..) |build_file_uri, index| {


How about have a loop with the following structure?
handle.associated_build_file = for (handle.potential_build_file_uris.items) { ... };

You can have a special check before for handle.potential_build_file_uris.items.len == 1. This should avoid having the boolean to reset the associated_build_file after the loop.

Techatrix · 2023-12-10T22:24:10Z

src/DocumentStore.zig

@@ -588,6 +600,32 @@ fn getOrLoadBuildFile(self: *DocumentStore, uri: Uri) ?*BuildFile {
    return gop.value_ptr.*;
 }

+pub fn doubleCheckPotentialBuildFileUri(self: *DocumentStore, handle: *Handle) error{OutOfMemory}!?Uri {


If we have a handle with multiple potential build files but none of them are associated to the handle, then this functions returns null. Every subsequent call will then do its entire work again and then still return null. There should be some kind of caching. This should also help with reducing contention on the handle mutex.

I'd say the issue is when there are none, otherwise if they all've been ran the first one sticks.

I'm thinking maybe passing an optional handle uri to getOrLoadBuildFile to do the association after the file has been ran (for every build file other than the first one). It'd be easier to pass a *Handle, but having an uri and calling getHandle on it ensures the handle is still valid.

Isn't it possible to return null when searching through multiple potential build files because of the if (!ok) handle.associated_build_file = null; line? If !ok then we return null and any subsequent call would also return null.

Equipped with the knowledge of how for() {} else works I think correcting the else to else true; should handle this condition;

llogick · 2023-12-18T23:26:49Z

Did the 'pass an optional handle uri to getOrLoadBuildFile to do the association after the file has been ran', but it had a blind spot where if you had a 'build.zig' file open it would be created without the "callback", would get loaded, but have no config when checked via the "callback", so a recheck in handle.getAssociatedBuildFileUri was needed.

Should we keep the "callback" complexity or have the "on demand" only logic?

Techatrix

Should we keep the "callback" complexity or have the "on demand" only logic?

I would consider your current "callback" approach to be acceptable (but a bit complex). I you want to do the "on demand" approach then this implementation of getAssociatedBuildFileUri could be feasible.

Keep in mind that I have not tested this code and that TODO should definitely be resolved.

pub fn getAssociatedBuildFileUri(self: *Handle, store: *DocumentStore) ?Uri {
    self.impl.lock.lock();
    defer self.impl.lock.unlock();
    switch(associated_build_file) {
        .none => return null,
        .unresolved => |potential_build_file_uris| {
            // should there be special logic if `potential_build_file_uris.len == 1`?
            
            var has_missing_build_config = false;
            for (potential_build_file_uris) |build_file_uri| {
                const build_file = self.getOrLoadBuildFile(build_file_uri, handle.uri) orelse continue; // continue or break?
                if (!build_file.hasConfig()) {
                    has_missing_build_config = true;
                    continue;
                }
                if (!try self.uriAssociatedWithBuild(build_file, handle.uri)) {
                    // TODO
                    // the given `build_file_uri` should be removed from `potential_build_file_uris`
                    // so that future calls don't redo all this work on every call.
                    continue;
                }
                /// the associated build file has been resolved so there is no need to store the potential build files.
                handle.potential_build_file_uris.clearAndFree(handle.allocator);
    
                associateBuildFileUri = .{ .resolved = build_file.uri };
                return build_file.uri;
            };
    
            if (!has_missing_build_config) {
                /// there is no associated build file so there is no need to store the potential build files.
                handle.potential_build_file_uris.clearAndFree(handle.allocator);
                associated_build_file = .none;
            }
            // when build configs are missing we keep the state at .unresolved so that
            // future calls will retry until all build config are resolved.
            // Then will have a conclusive result on whether or not there is a associated build file.
            return null;
        },
        .resolved => |uri| return uri,
    }
}

associated_build_file: union(enum) {
    /// The Handle has no associated build file (build.zig).
    none,
    /// The associated build file (build.zig) has not been resolved yet.
    /// Stores a list of potential build files.
    unresolved: []const Uri,
    /// The associated build file (build.zig) has been successfully resolved.
    resolved: Uri,
} = .unresolved,

Techatrix · 2023-12-24T23:22:18Z

src/DocumentStore.zig

-    if (handle.associated_build_file) |associated_build_file_uri| {
-        return std.mem.eql(u8, associated_build_file_uri, build_file_uri);
-    }


This has been added in #863 to make the build.zig discovery faster.
This PR should be checked against https://github.com/marlersoft/zigwin32.

Noice - a clear objective! ..also, I put those back in 😅, which restored same timings as main.
..but it was still slow (on my machine/fs), ..so I took it further 🤪

src/DocumentStore.zig

Techatrix · 2023-12-24T23:34:16Z

src/DocumentStore.zig

+    /// holds a `shared` lock on success
+    /// Release with `handle.unlockShared();`
+    pub fn getAssociatedBuildFileUri(self: *Handle, store: *DocumentStore, associate_if_pending: bool) ?Uri {
+        if (!self.impl.lock.tryLockShared()) return null;


why tryLockShared and not lockShared?

Most of the calls are for completions, so I didn't consider it necessary to succeed.

I will evaluate all the locks again, but I keep picturing a scenario where
I enter a ., don't get completions, bkspc, enter . x3 and have 3 threads blocked

Techatrix · 2023-12-24T23:56:29Z

src/DocumentStore.zig

+    /// holds a 'shared' lock on success
+    /// release with `build_file.unlockShared();`
+    pub fn getConfig(self: *BuildFile) ?BuildConfig {
+        if (!self.impl.lock.tryLockShared()) return null;


why tryLockShared and not lockShared?

src/DocumentStore.zig

Techatrix · 2023-12-25T00:29:36Z

src/DocumentStore.zig

+
+    handle.impl.associated_build_file = blk: for (handle.potential_build_file_uris.items, 0..) |build_file_uri, index| {
+        const build_file = self.getOrLoadBuildFile(build_file_uri, handle.uri) orelse break :blk .pending;
+        if (!build_file.hasConfig()) break :blk .pending;


Shouldn't it continue if there is a missing config?

But then it would reach the else => .none, unless I come around and implement your suggestion

src/DocumentStore.zig

llogick force-pushed the nullptrdevs/delay-finding-build-file-and-always-run-it branch from 3b2a85c to 6130513 Compare December 4, 2023 04:52

llogick marked this pull request as draft December 4, 2023 20:06

Techatrix self-requested a review December 5, 2023 17:36

Techatrix requested changes Dec 5, 2023

View reviewed changes

llogick force-pushed the nullptrdevs/delay-finding-build-file-and-always-run-it branch 2 times, most recently from be99b0f to f081c70 Compare December 9, 2023 11:22

llogick marked this pull request as ready for review December 9, 2023 11:28

llogick force-pushed the nullptrdevs/delay-finding-build-file-and-always-run-it branch from f081c70 to 7d27e60 Compare December 10, 2023 11:46

Techatrix requested changes Dec 10, 2023

View reviewed changes

llogick marked this pull request as draft December 18, 2023 16:54

llogick force-pushed the nullptrdevs/delay-finding-build-file-and-always-run-it branch from 7d27e60 to 738d0e5 Compare December 18, 2023 23:17

llogick force-pushed the nullptrdevs/delay-finding-build-file-and-always-run-it branch 3 times, most recently from 657fcda to b2740b2 Compare December 24, 2023 04:12

llogick marked this pull request as ready for review December 24, 2023 04:13

Techatrix requested changes Dec 25, 2023

View reviewed changes

llogick marked this pull request as draft December 26, 2023 02:58

llogick force-pushed the nullptrdevs/delay-finding-build-file-and-always-run-it branch 2 times, most recently from dc40e12 to e5ede4b Compare December 26, 2023 03:09

fix: wrong build file associated

340ac48

llogick force-pushed the nullptrdevs/delay-finding-build-file-and-always-run-it branch from e5ede4b to 340ac48 Compare December 28, 2023 14:56

llogick marked this pull request as ready for review December 28, 2023 16:26

llogick closed this Jan 8, 2024

llogick deleted the nullptrdevs/delay-finding-build-file-and-always-run-it branch February 4, 2024 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay finding and associating a build file with a (handle) uri #1647

Delay finding and associating a build file with a (handle) uri #1647

llogick commented Dec 3, 2023 •

edited

Loading

codecov bot commented Dec 3, 2023 •

edited

Loading

Techatrix Dec 5, 2023

Techatrix Dec 5, 2023

Techatrix Dec 5, 2023

Techatrix Dec 5, 2023

Techatrix Dec 5, 2023

Techatrix Dec 5, 2023

Techatrix commented Dec 5, 2023

llogick commented Dec 5, 2023

Techatrix commented Dec 5, 2023

Techatrix Dec 10, 2023

llogick Dec 10, 2023

Techatrix Dec 11, 2023

llogick Dec 11, 2023

Techatrix Dec 10, 2023

Techatrix Dec 10, 2023

Techatrix Dec 10, 2023

llogick Dec 10, 2023

Techatrix Dec 11, 2023 •

edited

Loading

llogick Dec 11, 2023

Techatrix Dec 10, 2023

Techatrix Dec 10, 2023

llogick Dec 10, 2023

Techatrix Dec 11, 2023

llogick Dec 11, 2023

llogick commented Dec 18, 2023

Techatrix left a comment

Techatrix Dec 24, 2023

llogick Dec 26, 2023

Techatrix Dec 24, 2023

llogick Dec 26, 2023 •

edited

Loading

Techatrix Dec 24, 2023

Techatrix Dec 25, 2023

llogick Dec 26, 2023

	return if (self.impl.config) \|_\| true else false;
	return self.impl.config != null;

Delay finding and associating a build file with a (handle) uri #1647

Delay finding and associating a build file with a (handle) uri #1647

Conversation

llogick commented Dec 3, 2023 • edited Loading

codecov bot commented Dec 3, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Techatrix commented Dec 5, 2023

llogick commented Dec 5, 2023

Techatrix commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Techatrix Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogick commented Dec 18, 2023

Techatrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogick Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogick commented Dec 3, 2023 •

edited

Loading

codecov bot commented Dec 3, 2023 •

edited

Loading

Techatrix Dec 11, 2023 •

edited

Loading

llogick Dec 26, 2023 •

edited

Loading