From 9a2695ecdb89de21e4ddf34e24a5bc440285edef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20H=C3=A4hne?= Date: Mon, 29 Mar 2021 11:21:39 +0200 Subject: [PATCH 1/2] Improve refreshDocument algorithm Do not use an arena, orderedRemove or bool array. Also, rudimentary tests suggest the config parser does not account for a substantial amount of the compile time. --- src/document_store.zig | 67 +++++++++++++++++------------------------- src/main.zig | 2 -- 2 files changed, 27 insertions(+), 42 deletions(-) diff --git a/src/document_store.zig b/src/document_store.zig index f563bb1..756a732 100644 --- a/src/document_store.zig +++ b/src/document_store.zig @@ -359,53 +359,40 @@ fn refreshDocument(self: *DocumentStore, handle: *Handle, zig_lib_path: ?[]const handle.document_scope.deinit(self.allocator); handle.document_scope = try analysis.makeDocumentScope(self.allocator, handle.tree); + + var new_imports = std.ArrayList([]const u8).init(self.allocator); + errdefer new_imports.deinit(); + try analysis.collectImports(&new_imports, handle.tree); - // TODO: Better algorithm or data structure? - // Removing the imports is costly since they live in an array list - // Perhaps we should use an AutoHashMap([]const u8, {}) ? - - // Try to detect removed imports and decrement their counts. - if (handle.import_uris.items.len == 0) return; - - var arena = std.heap.ArenaAllocator.init(self.allocator); - defer arena.deinit(); - - var import_strs = std.ArrayList([]const u8).init(&arena.allocator); - try analysis.collectImports(&import_strs, handle.tree); - - const still_exist = try arena.allocator.alloc(bool, handle.import_uris.items.len); - for (still_exist) |*ex| { - ex.* = false; + // Convert to URIs + var i: usize = 0; + while (i < new_imports.items.len) { + if (try self.uriFromImportStr(self.allocator, handle.*, new_imports.items[i])) |uri| { + // The raw import strings are owned by the document and do not need to be freed here. + new_imports.items[i] = uri; + i += 1; + } else { + _ = new_imports.swapRemove(i); + } } - const std_uri = try stdUriFromLibPath(&arena.allocator, zig_lib_path); - for (import_strs.items) |str| { - const uri = (try self.uriFromImportStr(&arena.allocator, handle.*, str)) orelse continue; + const old_imports = handle.import_uris; + handle.import_uris = new_imports; + defer old_imports.deinit(); - exists_loop: for (still_exist) |*does_still_exist, i| { - if (does_still_exist.*) continue; - - if (std.mem.eql(u8, handle.import_uris.items[i], uri)) { - does_still_exist.* = true; - break :exists_loop; + // Remove all old_imports that do not exist anymore + for (old_imports.items) |old| { + still_exists: { + for (new_imports) |new| { + if (std.mem.eql(u8, new, old)) { + break :still_exists; + } } + log.debug("Import removed: {s}", .{old}); + self.decrementCount(uri); + self.allocator.free(uri); } } - - // Go through still_exist, remove the items that are false and decrement their handle counts. - var idx: usize = 0; - for (still_exist) |does_still_exist| { - if (does_still_exist) { - idx += 1; - continue; - } - - log.debug("Import removed: {s}", .{handle.import_uris.items[idx]}); - const uri = handle.import_uris.orderedRemove(idx); - - self.decrementCount(uri); - self.allocator.free(uri); - } } pub fn applySave(self: *DocumentStore, handle: *Handle) !void { diff --git a/src/main.zig b/src/main.zig index 99cdcdd..c0e8df0 100644 --- a/src/main.zig +++ b/src/main.zig @@ -1213,8 +1213,6 @@ fn loadConfig(folder_path: []const u8) ?Config { }; defer allocator.free(file_buf); - // TODO: Uh oh. Profile the actual build time impact - // of adding config options and consider alternatives (TOML?) @setEvalBranchQuota(2000); // TODO: Better errors? Doesn't seem like std.json can provide us positions or context. var config = std.json.parse(Config, &std.json.TokenStream.init(file_buf), std.json.ParseOptions{ .allocator = allocator }) catch |err| { From ec1ee55dee4a721522bfa8aac13e0bda6bdaf936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20H=C3=A4hne?= Date: Mon, 29 Mar 2021 14:02:52 +0200 Subject: [PATCH 2/2] Accelerate symbolReferencesInternal by another factor of 2 and activate unit tests --- build.zig | 6 ++-- src/document_store.zig | 26 ++++++-------- {tests => src}/unit_tests.zig | 26 ++++++++++---- src/uri.zig | 67 ++++++++++++++++++++++++++++++----- 4 files changed, 92 insertions(+), 33 deletions(-) rename {tests => src}/unit_tests.zig (75%) diff --git a/build.zig b/build.zig index 458ac18..89523cd 100644 --- a/build.zig +++ b/build.zig @@ -237,14 +237,12 @@ pub fn build(b: *std.build.Builder) !void { const test_step = b.step("test", "Run all the tests"); test_step.dependOn(builder.getInstallStep()); - var unit_tests = b.addTest("tests/unit_tests.zig"); - unit_tests.addPackage(.{ .name = "analysis", .path = "src/analysis.zig" }); - unit_tests.addPackage(.{ .name = "types", .path = "src/types.zig" }); + var unit_tests = b.addTest("src/unit_tests.zig"); unit_tests.setBuildMode(.Debug); + test_step.dependOn(&unit_tests.step); var session_tests = b.addTest("tests/sessions.zig"); session_tests.addPackage(.{ .name = "header", .path = "src/header.zig" }); session_tests.setBuildMode(.Debug); - test_step.dependOn(&session_tests.step); } diff --git a/src/document_store.zig b/src/document_store.zig index 756a732..c67cbba 100644 --- a/src/document_store.zig +++ b/src/document_store.zig @@ -383,14 +383,14 @@ fn refreshDocument(self: *DocumentStore, handle: *Handle, zig_lib_path: ?[]const // Remove all old_imports that do not exist anymore for (old_imports.items) |old| { still_exists: { - for (new_imports) |new| { + for (new_imports.items) |new| { if (std.mem.eql(u8, new, old)) { break :still_exists; } } log.debug("Import removed: {s}", .{old}); - self.decrementCount(uri); - self.allocator.free(uri); + self.decrementCount(old); + self.allocator.free(old); } } } @@ -500,18 +500,14 @@ pub fn uriFromImportStr( } return null; } else { - // Find relative uri - const path = try URI.parse(allocator, handle.uri()); - defer allocator.free(path); - - const dir_path = std.fs.path.dirname(path) orelse ""; - const import_path = try std.fs.path.resolve(allocator, &[_][]const u8{ - dir_path, import_str, - }); - - defer allocator.free(import_path); - - return try URI.fromPath(allocator, import_path); + const base = handle.uri(); + var base_len = base.len; + while (base[base_len - 1] != '/' and base_len > 0) { base_len -= 1; } + base_len -= 1; + if (base_len <= 0) { + return error.UriBadScheme; + } + return try URI.pathRelative(allocator, base[0..base_len], import_str); } } diff --git a/tests/unit_tests.zig b/src/unit_tests.zig similarity index 75% rename from tests/unit_tests.zig rename to src/unit_tests.zig index cde0373..8bc4814 100644 --- a/tests/unit_tests.zig +++ b/src/unit_tests.zig @@ -1,5 +1,7 @@ -const analysis = @import("analysis"); -const types = @import("types"); +const analysis = @import("analysis.zig"); +const types = @import("types.zig"); +const offsets = @import("offsets.zig"); +const URI = @import("uri.zig"); const std = @import("std"); @@ -30,11 +32,11 @@ fn testContext(comptime line: []const u8, comptime tag: anytype, comptime range: const doc = try makeUnnamedDocument(final_line); defer freeDocument(doc); + var arena = std.heap.ArenaAllocator.init(allocator); + defer arena.deinit(); - const ctx = try analysis.documentPositionContext(allocator, doc, types.Position{ - .line = 0, - .character = @intCast(i64, cursor_idx), - }); + const p = try offsets.documentPosition(doc, .{ .line = 0, .character = @intCast(i64, cursor_idx) }, .utf8); + const ctx = try analysis.documentPositionContext(&arena, doc, p); if (std.meta.activeTag(ctx) != tag) { std.debug.warn("Expected tag {}, got {}\n", .{ tag, std.meta.activeTag(ctx) }); @@ -100,3 +102,15 @@ test "documentPositionContext" { "Se", ); } + + +test "pathRelative and escapes" { + const join1 = try URI.pathRelative(allocator, "file://project/zig", "/src/main+.zig"); + defer allocator.free(join1); + std.testing.expectEqualStrings("file://project/zig/src/main%2B.zig", join1); + + const join2 = try URI.pathRelative(allocator, "file://project/zig/wow", "../]src]/]main.zig"); + defer allocator.free(join2); + std.testing.expectEqualStrings("file://project/zig/%5Dsrc%5D/%5Dmain.zig", join2); +} + diff --git a/src/uri.zig b/src/uri.zig index 1e7c89a..a08c7eb 100644 --- a/src/uri.zig +++ b/src/uri.zig @@ -1,13 +1,24 @@ const std = @import("std"); +const mem = std.mem; const reserved_chars = &[_]u8{ '!', '#', '$', '%', '&', '\'', '(', ')', '*', '+', ',', ':', ';', '=', '?', '@', '[', ']', }; +const reserved_escapes = comptime blk: { + var escapes: [reserved_chars.len][3]u8 + = [_][3]u8{[_]u8{undefined} ** 3} ** reserved_chars.len; + + for (reserved_chars) |c, i| { + escapes[i][0] = '%'; + _ = std.fmt.bufPrint(escapes[i][1..], "{X}", .{c}) catch unreachable; + } + break :blk &escapes; +}; /// Returns a URI from a path, caller owns the memory allocated with `allocator` -pub fn fromPath(allocator: *std.mem.Allocator, path: []const u8) ![]const u8 { +pub fn fromPath(allocator: *mem.Allocator, path: []const u8) ![]const u8 { if (path.len == 0) return ""; const prefix = if (std.builtin.os.tag == .windows) "file:///" else "file://"; @@ -19,10 +30,8 @@ pub fn fromPath(allocator: *std.mem.Allocator, path: []const u8) ![]const u8 { for (path) |char| { if (char == std.fs.path.sep) { try buf.append('/'); - } else if (std.mem.indexOfScalar(u8, reserved_chars, char) != null) { - // Write '%' + hex with uppercase - try buf.append('%'); - try std.fmt.format(out_stream, "{X}", .{char}); + } else if (mem.indexOfScalar(u8, reserved_chars, char)) |reserved| { + try buf.appendSlice(&reserved_escapes[reserved]); } else { try buf.append(char); } @@ -32,7 +41,7 @@ pub fn fromPath(allocator: *std.mem.Allocator, path: []const u8) ![]const u8 { if (std.builtin.os.tag == .windows) { if (buf.items.len > prefix.len + 1 and std.ascii.isAlpha(buf.items[prefix.len]) and - std.mem.startsWith(u8, buf.items[prefix.len + 1 ..], "%3A")) + mem.startsWith(u8, buf.items[prefix.len + 1 ..], "%3A")) { buf.items[prefix.len] = std.ascii.toLower(buf.items[prefix.len]); } @@ -41,6 +50,47 @@ pub fn fromPath(allocator: *std.mem.Allocator, path: []const u8) ![]const u8 { return buf.toOwnedSlice(); } +/// Move along `rel` from `base` with a single allocation. +/// `base` is a URI of a folder, `rel` is a raw relative path. +pub fn pathRelative(allocator: *mem.Allocator, base: []const u8, rel: []const u8) ![]const u8 { + const max_size = base.len + rel.len * 3 + 1; + + var result = try allocator.alloc(u8, max_size); + errdefer allocator.free(result); + mem.copy(u8, result, base); + var result_index: usize = base.len; + + var it = mem.tokenize(rel, "/"); + while (it.next()) |component| { + if (mem.eql(u8, component, ".")) { + continue; + } else if (mem.eql(u8, component, "..")) { + while (true) { + if (result_index == 0) + return error.UriBadScheme; + result_index -= 1; + if (result[result_index] == '/') + break; + } + } else { + result[result_index] = '/'; + result_index += 1; + for (component) |char| { + if (mem.indexOfScalar(u8, reserved_chars, char)) |reserved| { + const escape = &reserved_escapes[reserved]; + mem.copy(u8, result[result_index..], escape); + result_index += escape.len; + } else { + result[result_index] = char; + result_index += 1; + } + } + } + } + + return allocator.resize(result, result_index); +} + pub const UriParseError = error{ UriBadScheme, UriBadHexChar, @@ -59,8 +109,8 @@ fn parseHex(c: u8) !u8 { } /// Caller should free memory -pub fn parse(allocator: *std.mem.Allocator, str: []const u8) ![]u8 { - if (str.len < 7 or !std.mem.eql(u8, "file://", str[0..7])) return error.UriBadScheme; +pub fn parse(allocator: *mem.Allocator, str: []const u8) ![]u8 { + if (str.len < 7 or !mem.eql(u8, "file://", str[0..7])) return error.UriBadScheme; const uri = try allocator.alloc(u8, str.len - (if (std.fs.path.sep == '\\') 8 else 7)); errdefer allocator.free(uri); @@ -89,3 +139,4 @@ pub fn parse(allocator: *std.mem.Allocator, str: []const u8) ![]u8 { return allocator.shrink(uri, i); } +