Skip to content

Conversation

b1ek
Copy link
Member

@b1ek b1ek commented Nov 15, 2024

scratch implementation of caching parsed blocks to speed up performance with libraries

@b1ek b1ek added enhancement New feature or request compiler labels Nov 15, 2024
@b1ek b1ek self-assigned this Nov 15, 2024
@Mte90
Copy link
Member

Mte90 commented Nov 15, 2024

I can't wait for this :-D

@Ph0enixKM
Copy link
Member

I think I understand what this PR adds. @b1ek could you at least create an issue for it so that we have a clear definition of what this PR solves?

@b1ek
Copy link
Member Author

b1ek commented Nov 19, 2024

create an issue for it

its #524, but needs to be edited a little bit

@Ph0enixKM Ph0enixKM changed the base branch from master to staging December 19, 2024 11:13
@Mte90
Copy link
Member

Mte90 commented Apr 10, 2025

@b1ek requires some updates

@b1ek b1ek requested a review from Copilot July 19, 2025 07:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request implements a preparsed cache system to improve compilation performance by caching both tokenized and parsed representations of Amber source files. The caching system supports multiple file sources (File, Stdlib, Stream) with appropriate invalidation based on file modification times and Git hash changes.

  • Adds comprehensive serialization support across the entire AST and metadata structures
  • Implements tokenization and parsing cache with file-based storage
  • Introduces file metadata system to differentiate between file sources and control cache behavior

Reviewed Changes

Copilot reviewed 80 out of 84 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/compiler/cache/tokenize.rs Implements tokenization cache with file validation and storage
src/compiler/cache/parse.rs Implements parsing cache with metadata serialization
src/compiler/cache.rs Cache infrastructure and home directory management
src/compiler/file_source.rs File metadata system for cache control
src/compiler.rs Integration of caching into the compilation pipeline
src/stdlib.rs Precompilation functionality for standard library
Multiple module files Addition of Serialize/Deserialize traits to AST structures
Comments suppressed due to low confidence (3)

src/stdlib.rs:62

  • The variable name 'i' is ambiguous. Consider renaming it to 'completed_count' or 'processed_files' to better reflect its purpose as a counter for completed thread operations.
    let mut i = 0;

src/compiler/cache/tokenize.rs:16

  • [nitpick] The file extension 'ab.tk.c' could be more descriptive. Consider using 'amber_tokens_cache' or 'atk' to make the purpose clearer.
pub const FILE_EXT: &'static str = "ab.tk.c";

src/compiler/cache/parse.rs:8

  • [nitpick] The file extension 'ab.pr.c' could be more descriptive. Consider using 'amber_parse_cache' or 'apc' to make the purpose clearer.
pub const FILE_EXT: &'static str = "ab.pr.c";

fs::write(&cache_file, serialized).map_err(|x| format!("Cannot write to {cache_file:?}: {x}"))?;

#[cfg(unix)]
fs::set_permissions(&filename, Permissions::from_mode(0o700)).map_err(|x| format!("Cannot set perms to {cache_file:?}: {x}"))?;
Copy link
Preview

Copilot AI Jul 19, 2025

Choose a reason for hiding this comment

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

The code is setting permissions on the original filename instead of the cache_file. This should be 'fs::set_permissions(&cache_file, ...)' to properly secure the cache file.

Suggested change
fs::set_permissions(&filename, Permissions::from_mode(0o700)).map_err(|x| format!("Cannot set perms to {cache_file:?}: {x}"))?;
fs::set_permissions(&cache_file, Permissions::from_mode(0o700)).map_err(|x| format!("Cannot set perms to {cache_file:?}: {x}"))?;

Copilot uses AI. Check for mistakes.

fs::set_permissions(&filename, Permissions::from_mode(0o700)).map_err(|x| format!("Cannot set perms to {cache_file:?}: {x}"))?;

#[cfg(not(unix))] {
let _ = hf::hide(&filename);
Copy link
Preview

Copilot AI Jul 19, 2025

Choose a reason for hiding this comment

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

Similar to the permissions issue, this is hiding the original filename instead of the cache_file. This should be 'hf::hide(&cache_file)' and 'hf' appears to be undefined.

Copilot uses AI. Check for mistakes.

fs::set_permissions(&filename, Permissions::from_mode(0o700)).map_err(|x| format!("Cannot set perms to {filename:?}: {x}"))?;

#[cfg(not(unix))] {
let _ = hf::hide(&filename);
Copy link
Preview

Copilot AI Jul 19, 2025

Choose a reason for hiding this comment

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

The identifier 'hf' is not defined or imported. This will cause a compilation error.

Copilot uses AI. Check for mistakes.

let caught = std::panic::catch_unwind(|| {
precompile_thread(file.clone(), tx.clone());
});
if caught.is_err() {
Copy link
Preview

Copilot AI Jul 19, 2025

Choose a reason for hiding this comment

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

Consider logging or handling the panic information from 'caught' to help with debugging. The current implementation silently converts all panics to "error".

Copilot uses AI. Check for mistakes.


use crate::compiler::cache::GIT_HASH;
use crate::compiler::file_source::{FileMeta, FileSource};
use crate::modules::expression::unop::not;
Copy link
Preview

Copilot AI Jul 19, 2025

Choose a reason for hiding this comment

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

This import appears to be unused in the file. Consider removing it to keep the imports clean.

Suggested change
use crate::modules::expression::unop::not;

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants