Skip to content

Only log build settings once unless they change #932

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 27, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 53 additions & 36 deletions Sources/SKCore/BuildSystemManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -72,25 +72,6 @@ public actor BuildSystemManager {
}
}

/// Splits `message` on newline characters such that each chunk is at most `maxChunkSize` bytes long.
///
/// The intended use case for this is to split compiler arguments into multiple chunks so that each chunk doesn't exceed
/// the maximum message length of `os_log` and thus won't get truncated.
///
/// - Note: This will only split along newline boundary. If a single line is longer than `maxChunkSize`, it won't be
/// split. This is fine for compiler argument splitting since a single argument is rarely longer than 800 characters.
private func splitLongMultilineMessage(message: String, maxChunkSize: Int) -> [String] {
var chunks: [String] = []
for line in message.split(separator: "\n", omittingEmptySubsequences: false) {
if let lastChunk = chunks.last, lastChunk.utf8.count + line.utf8.count < maxChunkSize {
chunks[chunks.count - 1] += "\n" + line
} else {
chunks.append(String(line))
}
}
return chunks
}

extension BuildSystemManager {
public var delegate: BuildSystemDelegate? {
get { _delegate }
Expand Down Expand Up @@ -160,23 +141,7 @@ extension BuildSystemManager {
// to reference `document` instead of `mainFile`.
settings = settings.patching(newFile: document.pseudoPath, originalFile: mainFile.pseudoPath)
}
let log = """
Compiler Arguments:
\(settings.compilerArguments.joined(separator: "\n"))

Working directory:
\(settings.workingDirectory ?? "<nil>")
"""

let chunks = splitLongMultilineMessage(message: log, maxChunkSize: 800)
for (index, chunk) in chunks.enumerated() {
logger.log(
"""
Build settings for \(document.forLogging) (\(index + 1)/\(chunks.count))
\(chunk)
"""
)
}
await BuildSettingsLogger.shared.log(settings: settings, for: document)
return settings
}

Expand Down Expand Up @@ -334,3 +299,55 @@ extension BuildSystemManager {
return self.watchedFiles[uri]?.mainFile
}
}

// MARK: - Build settings logger

/// Shared logger that only logs build settings for a file once unless they change
fileprivate actor BuildSettingsLogger {
static let shared = BuildSettingsLogger()

private var loggedSettings: [DocumentURI: FileBuildSettings] = [:]
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we likely already keep a document -> settings in memory for all the workspaces, but it would be nice to not (potentially) double it. Are you worried about some race if this was removed on close?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK we don’t keep that mapping around anywhere. We used to but I removed the caches in SwiftLanguageServer and ClangLanguageServer.

SwiftPMWorkspace, for example, gets the build settings from the build graph and doesn’t have any file -> build settings cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m merging this now. If you think this should be implemented otherwise, I can address it in a follow-up PR.


func log(settings: FileBuildSettings, for uri: DocumentURI) {
guard loggedSettings[uri] != settings else {
return
}
loggedSettings[uri] = settings
let log = """
Compiler Arguments:
\(settings.compilerArguments.joined(separator: "\n"))

Working directory:
\(settings.workingDirectory ?? "<nil>")
"""

let chunks = splitLongMultilineMessage(message: log, maxChunkSize: 800)
for (index, chunk) in chunks.enumerated() {
logger.log(
"""
Build settings for \(uri.forLogging) (\(index + 1)/\(chunks.count))
\(chunk)
"""
)
}
}

/// Splits `message` on newline characters such that each chunk is at most `maxChunkSize` bytes long.
///
/// The intended use case for this is to split compiler arguments into multiple chunks so that each chunk doesn't exceed
/// the maximum message length of `os_log` and thus won't get truncated.
///
/// - Note: This will only split along newline boundary. If a single line is longer than `maxChunkSize`, it won't be
/// split. This is fine for compiler argument splitting since a single argument is rarely longer than 800 characters.
private func splitLongMultilineMessage(message: String, maxChunkSize: Int) -> [String] {
var chunks: [String] = []
for line in message.split(separator: "\n", omittingEmptySubsequences: false) {
if let lastChunk = chunks.last, lastChunk.utf8.count + line.utf8.count < maxChunkSize {
chunks[chunks.count - 1] += "\n" + line
} else {
chunks.append(String(line))
}
}
return chunks
}
}