Skip to content
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

Improve Skip Builds Handling on Build CLI & Disable TLS Initial on Jemalloc #2154

Merged
merged 7 commits into from
Dec 11, 2024

Conversation

AmmarAbouZor
Copy link
Member

This PR closes #2152

Build CLI Tool:

This PR improves tracking the state of the last build to determine which builds can be skipped by:

  • Additional features will be included in the persists build records for the last running job alongside with Checksum of the files for the given target.
  • Use serde to write and parse the file instead of the manually implementation previously, enabling adding more information to the build state without having to change the parse code.
  • Use one file for development and production by saving this information in the code itself instead of having to files for each build since we will keep one of them at a time anyway.
  • Updating the integration tests for the CLI tool to accommodate the new changes.

Jemalloc Memory Allocator:

This PR disable initial allocating TLS memory because it leads to Chipmunk exceeding the allowed TLS memory for a process on Linux machine when it run combined with NodeJS and Electron

@AmmarAbouZor AmmarAbouZor requested a review from marcmo November 21, 2024 14:55
* Use serde for loading & saving the file instead of the manual parsing
* Additional features on the last run are involved in the persisted
  states
* Use one file for states instead of two for development and production
  by including the production state in the file itself
* Rename the module and structs for the build records
* Move additional features to thier own module
* Add new build state file name to gitignore
Adjust the integration tests with the new changes on the reset command
Jemalloc combined with Node.js exceeds the default TLS memory limit on Linux.
Comment on lines 39 to 41
pub struct BuildStateRecords {
items: Mutex<BuildStateItems>,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bellow in code you are getting access to BuildStateRecords as a ref

 pub fn get(production: bool) -> anyhow::Result<&'static BuildStateRecords> {
        static CHECKSUM_RECORDS: OnceLock<anyhow::Result<BuildStateRecords>> = OnceLock::new();

        CHECKSUM_RECORDS
            .get_or_init(|| BuildStateRecords::load(production))
            .as_ref()
            .map_err(|err| anyhow!("{err}"))
    }

All looks good, but it's still a little bit weird. You are wrapping items into Mutex to have mut access to a list, all good. But what it in some time letter you will need some new member in BuildStateRecords, which also would need mut access?

Would it make the solution more scalable, if in OnceLock you will create not an instance of BuildStateRecords, but Mutex<BuildStateRecords>. In this case, you would not need to wrap items into Mutex and you will be able to get access to &mut BuildStateRecords from everywhere. Besides that, you BuildStateRecords can be easily extended with a new field, if it will be needed. Also good alternative might be RwLock if not all calls of BuildStateRecords require mut.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AmmarAbouZor to me looks good and can be merged.. "green" light to do it. I've left only one comment and leave it to your responsibility. Realization of BuildStateRecords, looks good, but not scalable. Suggested changes will not change how the solution works, but can make it just more scalable. Maybe only one thing... If you never going to extend BuildStateRecords, probably makes sense to declare it as TupleStruct BuildStateRecords(BuildStateItems) to make it more transparent for a developer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very thanks @DmitryAstafyev for pointing out the issue with internal Mutex. At the start I thought that we could have other fields which doesn't need to be wrapped within the Mutex, but it ended up with more complex code. I moved the Mutex to outside of the struct itself.
This change won't remove the as_ref() call because this call is to avoid wrapping the Mutex within an Arc since we always need a reference of the records.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AmmarAbouZor thanks for the corrections. Let it go! You are able to merge it as well

Wrap whole checksum manager inside a mutex instead of hiding it inside
Remove inside items wrapper and set their fields directly inside the
manager since we are not wrapping the inner content with a mutex anymore
@DmitryAstafyev DmitryAstafyev merged commit 029d7d3 into esrlabs:master Dec 11, 2024
2 checks passed
@AmmarAbouZor AmmarAbouZor deleted the build_cli_feat_skip branch December 11, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build CLI: Include Additional Features in Skip Checks
2 participants