Reduce usage of clone() #787
Comments
|
@YamatoSecurity |
|
Ok, thanks! Do what you can and if there are some difficult ones to fix we can always ask others for help. |
|
The result of grepping v1.7.2 (files) for "clone()" is as follows.
|
|
removed following clone in ec3a902
|
|
I will continue to investigate the following points that can be expected to reduce memory 🤔
|
|
Thank you for listing it.
I agree that these responses are necessary.
Regular expressions are difficult to deal with in this project because they must be used if they are written with sigma rules. |
|
Thanks for the infomation as stated above :) I understood the difficulty of ↑ regex memory reduction >< ! |
|
以下構造体の2フィールドを削除(と元のデータを持っているrecordを必要な時に参照させる形に変更)することで、 mainブランチと比較して、メモリ使用量(peak reservedで比較)を少しだけ↓削減できました
(mimallocの統計結果より、実測値のメモリ改善幅はもう少し小さく、200MB程度になりますが... ) -P standard の比較e873adf (mainブランチ) a063722 (devブランチ) -P super-verbose の比較e873adf (mainブランチ) a063722 (devブランチ) 今週中は、v2.0機能のマージと検証で大変お忙しいと思いますので、v2リリース作業が落ち着きましたら、お時間あるときに↑内容見ていただけますと幸いです🙇 |
|
@fukusuket ありがとうございます。確認致しました。実装面は特に問題ないと思います。 |
|
早々にご確認ありがとうございます！🙇 |
|
@fukusuket ありがとうございます！ |
|
早々に検証いただき、ありがとうございます！(改善幅小さく、他データでも改善するかわからなかったので、大変助かります🙇) v2機能マージ完了後に、mainからbranch作成しまして、こちらのPRお送りいたします！ |
|
@fukusuket |
|
@fukusuket I uploaded Use of Reference Instead of Cloning: Try to pass references to functions and methods whenever possible, and avoid cloning unless it's necessary. For example, in the following line, you seem to be passing a reference to the stored_static object:
Reduce the Use of unwrap(): The unwrap() function is used quite frequently in your code. Each use of unwrap() can potentially cause a panic if the Result or Option is an Err or None, respectively. This can cause the whole program to crash. Preferably, handle such cases explicitly using pattern matching or methods like unwrap_or_else, unwrap_or, unwrap_or_default, etc. Reuse Computed Values: It appears that there are multiple instances where you compute the same thing, such as stored_static.output_option.as_ref().unwrap(). Storing this in a variable and reusing it could be beneficial. Early Return: If you are checking for error conditions and returning early, consider structuring your code such that you check for the "happy path" and proceed, returning early in the case of error conditions. This can help reduce the level of indentation in your code, making it easier to read. Avoid Unnecessary Heap Allocations: Methods like to_string(), format!(), etc. create a new heap-allocated string. If the string isn't needed, consider avoiding these methods. Lazy Evaluation: In some cases, like formatting strings or other potentially expensive operations, it might be beneficial to use Rust's lazy evaluation features. Instead of formatting a string immediately, you can prepare the format string and arguments and only actually format the string if it is needed. Efficient Use of Collections: Ensure you're using the most efficient collection type for your needs. For example, if you're working with a set of items where order doesn't matter but you want quick lookups, a HashSet might be a better choice than a Vec. Avoid Recomputing Paths: In your current code, you are calling check_setting_path() method multiple times for the same path. You should avoid doing this as it could be a performance issue if the method is resource-intensive. Instead, store the result in a variable and reuse it. Remove Unnecessary Clones: Check if there are any unnecessary clones in your code. For example, to_string() also creates a new String object. So, you may be able to replace to_string() with as_str() in some cases to avoid creating a new String. Lazy Computation: In cases where you use unwrap_or and unwrap_or_else, if the computation of the default value is expensive, use unwrap_or_else as it will not compute the value unless it is actually required. Avoid Unnecessary Unwraps: There seems to be quite a few places in the code where unwrap() is being used. This can lead to a runtime panic if the Result or Option is an Err or None respectively. Instead, consider handling these cases explicitly or using something like unwrap_or or unwrap_or_else to provide a default. Efficient Use of Collections: When you are using collections like vectors or hashmaps, try to specify the capacity if it is known in advance. This will prevent reallocations as the collection grows. If the collection is large, consider using more efficient data structures or algorithms. Reducing Syscalls: If your program does a lot of reading from or writing to files, network etc., try to do these operations in larger batches to reduce the number of system calls, which can be quite expensive. Remember, the first rule of optimization is: Don't do it; the second rule of optimization (for experts only) is: Don't do it yet. Measure and identify your performance bottlenecks before you try to optimize anything. Micro-optimizations like these can make your code more complex, and often provide little to no real-world performance benefits. It's usually better to focus on high-level architecture and algorithmic efficiency. Shared Reference Instead of Cloning: It seems that target_extensions is a borrowed HashSet in analysis_start. If you're passing this collection around but not modifying it, consider using a borrowed reference in the first place. This will save on the cost of cloning the entire HashSet every time the function is called. Concurrency: The analysis_start method seems like a place where some concurrency could potentially be introduced. If the analysis of different extensions is independent, you could potentially perform these operations in parallel using threads. However, be aware that introducing concurrency could make the code more complex and also introduces overhead. This should be considered only if the analysis_start method is a major bottleneck and the amount of data is large enough to offset the threading overhead. Avoid Locking Where Possible: There is a line where you lock ERROR_LOG_STACK to check its length. If you often need to check whether the error log stack is empty, and if this lock gets contended, you could consider keeping an atomic counter alongside the stack. That way, you could check the counter instead of having to acquire the lock. Minimize Heap Allocation: Calling methods like format!() often can lead to many small heap allocations, which can be slow. If you can, try to replace some of these calls with methods that format data directly into an output buffer, like write!() or writeln!(). You could also consider using a library like itoa for faster integer to string conversion. Reusing Buffers: If you are frequently creating and dropping buffers, that can lead to a lot of heap allocation/deallocation churn, which can slow down your program. Consider reusing buffers where possible. Early Return and Avoiding Clones: In the if condition block where you check if stored_static.output_option is None, you can early return without any further processing. This can help avoid unnecessary cloning and function calls. Avoid Unnecessary String Manipulation: In the code block where you remove the leading and trailing double quotes from replaced_filepath, you can optimize it by using methods like trim_start_matches('"') and trim_end_matches('"') to remove the quotes. This avoids unnecessary cloning of the string and makes the code more concise. Reduce Function Calls: In the else if block where you check for live_analysis and directory, you are calling the collect_liveanalysis_files and collect_evtxfiles methods. Consider storing the return values of these methods in variables and reusing them instead of calling the methods multiple times. Avoid Cloning Strings: Instead of cloning the filepath string, you can directly work with a borrowed reference to avoid unnecessary cloning. You can change the following line:
In the analysis_start method: Avoid String Manipulation: Instead of manipulating the replaced_filepath string by removing the leading and trailing quotes, you can use the strip_prefix() and strip_suffix() methods to remove the quotes more efficiently. Here's an example: This avoids unnecessary cloning and string manipulation. Avoid Unnecessary String Conversions: In the line where you check if the file extension is in target_extensions, you can avoid converting the extension to a string by comparing it directly to a borrowed string slice. Here's an example:
Reduce Function Calls: In the else if block where you call collect_liveanalysis_files, consider storing the return value of the method in a variable instead of calling it twice. This can help reduce unnecessary function calls and improve code readability. Regarding the collect_liveanalysis_files method: Early Return: In the #[cfg(not(target_os = "windows"))] branch, you can early return None instead of printing an alert message. This can simplify the code and avoid unnecessary output. Unused Parameters: In the #[cfg(target_os = "windows")] branch, if the target_extensions and stored_static parameters are not used, you can add an underscore (_) prefix to their names to indicate that they are unused. This makes the code clearer and avoids compiler warnings. In the collect_evtxfiles method: Avoid String Manipulation: Similar to the previous optimization, you can use the strip_prefix() and strip_suffix() methods to remove the leading and trailing quotes from dirpath more efficiently. Here's an example: This avoids unnecessary cloning and string manipulation. Use PathBuf for Directory Paths: Instead of using a String for dirpath, you can use a PathBuf to handle directory paths. This provides better type safety and avoids unnecessary string conversions. Here's an example: This improves code readability and avoids unnecessary string operations. Use read_dir() Iterator: Instead of calling read_dir() twice (once to check for errors and once to iterate over the directory entries), you can directly iterate over the entries iterator returned by read_dir(). This eliminates the need for the is_err() check. Here's an example: This simplifies the code and removes unnecessary error checking. These optimizations should help improve performance and code readability in the collect_evtxfiles method. In the analysis_files method: Use matches! Macro: Instead of using multiple to_uppercase() calls and == comparisons, you can use the matches! macro to simplify the level comparison. Here's an example: This simplifies the level comparison logic and improves code readability. Use path.file_name(): Instead of converting the PathBuf to a string and then parsing the file name, you can use the file_name() method to directly access the file name as an OsStr. Here's an example: This eliminates the need for unnecessary string conversions. These optimizations should improve the performance and readability of the analysis_files method. In the analysis_files method: Avoid Unnecessary String Formatting: Instead of formatting the total_size_output string and immediately printing it, you can directly print the total file size using total_file_size.to_string_as(false) without assigning it to a variable. This eliminates the need for the intermediate total_size_output string. Here's an example:
Avoid Redundant println!() Calls: Instead of printing empty lines using multiple println!() calls, you can use println!() once with multiple newlines (\n) to achieve the same effect. Here's an example:
Avoid Redundant Cloning: Instead of cloning the stored_static variable multiple times, you can pass it as a shared reference &stored_static to the functions where it is needed. This avoids unnecessary cloning and improves performance. Here's an example: Avoid Unnecessary Cloning of tl: Instead of cloning the tl variable in each iteration of the loop, you can initialize it once before the loop and then pass a mutable reference &mut tl to the functions. This avoids unnecessary cloning. Here's an example: By applying these optimizations, you can reduce unnecessary cloning and improve the performance and memory efficiency of the code. In the analysis_file method: Avoid Redundant Cloning: Instead of cloning the stored_static variable multiple times, you can pass it as a shared reference &stored_static to the functions where it is needed. This avoids unnecessary cloning and improves performance. Here's an example:
output_and_data_stack_for_html(&output_str, "General Overview {#general_overview}", &stored_static.html_report_flag) This eliminates the need for the inner while loop and the break statement. Avoid Unnecessary Variable Assignment: The record_result variable can be directly used instead of assigning it to another variable let next_rec = record_result;. This reduces unnecessary variable assignments and improves code readability. In the analysis_file method: Avoid Unnecessary Variable Assignment: The data variable can be directly used instead of assigning it to another variable let data = record_result.as_ref().unwrap().data;. This reduces unnecessary variable assignments and improves code readability. Here's an example: This eliminates the need for the data variable. Avoid Cloning the Data: Instead of cloning the data vector and pushing it into records_per_detect, you can directly pass a reference &data to records_per_detect. This avoids unnecessary cloning and improves performance. Here's an example:
Avoid Unnecessary Function Call: Instead of calling to_owned() on tl when passing it to the analysis_json_record function, you can pass a shared reference &tl directly. This avoids unnecessary cloning and improves performance. Here's an example: Make sure to adjust the function signature of analysis_json_record to accept a reference to tl instead of an owned value. Avoid Unnecessary Function Arguments: Since stored_static and target_event_ids are already part of the self struct, you don't need to pass them as separate function arguments to the analysis_json_record function. You can directly access them using self.stored_static and self.target_event_ids. This simplifies the function signature and eliminates unnecessary function arguments. Use while let Loop Instead of loop: Instead of using a loop and manually breaking the loop, you can use a while let loop to iterate over the records. This eliminates the need for the inner break statement. Here's an example: This ensures that the loop exits automatically when there are no more records to process. In the analysis_json_file method: Avoid Unnecessary Variable Assignment: The jsonl_value_iter variable can be directly used instead of assigning it to another variable let mut records = match jsonl_value_iter {...};. This eliminates the need for the records variable. Here's an example: This simplifies the code and reduces unnecessary variable assignments. Use while let Loop Instead of loop: Similar to the previous optimization, you can use a while let loop instead of a loop to iterate over the records. This eliminates the need for the inner break statement. Here's an example: This ensures that the loop exits automatically when there are no more records to process. Avoid Unnecessary Cloning: Instead of cloning the filepath variable to create filepath again, you can directly use filename and CURRENT_EXE_PATH to construct the filepath string. This eliminates the need for the unnecessary cloning. Here's an example: This simplifies the code and improves performance. Avoid Unnecessary Cloning: The code includes several cloning operations, such as data["Event"]["EventData"].clone() and data["Event"]["EventData"]["RecordNumber"].clone(). If possible, try to minimize the need for cloning by reorganizing the code or using references where applicable. Use if let Instead of unwrap(): Instead of using unwrap() followed by checking if the value is None, you can use if let to directly handle the Some case and ignore the None case. This can make the code more concise. For example: can be simplified to: This eliminates the need for the explicit check for None and reduces unnecessary operations. Avoid Mutating the Data In Place: The code modifies the data object by inserting and updating values. Consider whether it's necessary to modify the data in place or if a more functional approach can be used, which avoids mutations and provides clearer code. If the modifications are required, make sure they are done in an efficient manner to minimize unnecessary operations. Reuse String Buffers: Instead of creating a new string buffer for each iteration in the timestamp parsing step, consider reusing a single string buffer. This can reduce memory allocations and improve performance. You can use the String::clear() method to clear the buffer and then append new values in each iteration. Use Early Returns: Instead of using an if statement to check if records_per_detect is empty at the end of the loop, you can use an early return statement to exit the loop immediately when no records are found. This can save unnecessary iterations and improve performance. For example: can be replaced with: This way, the loop will terminate early when there are no records, avoiding unnecessary iterations. Optimize Timestamp Parsing: Timestamp parsing can be a costly operation, especially when performed repeatedly. Consider optimizing the timestamp parsing step by leveraging a more efficient parsing library or using a custom parsing function that avoids unnecessary string replacements. Additionally, you can consider using a thread-local pool of parsers to avoid repeated allocations. Preprocess Target Event IDs: Instead of processing the target_event_ids configuration for each event record, preprocess the configuration to create a set or a hashmap of target event IDs. This preprocessing step can be done once at the beginning, reducing the lookup time for each event record. Use HashSet for Event ID Filtering: Instead of iterating over a list to check if an event ID is a target event ID, use a HashSet or a HashMap for efficient event ID lookups. The HashSet or HashMap can be created during the preprocessing step mentioned above. Use Borrowed References: Instead of passing owned values such as &stored_static.eventkey_alias as arguments to the filtering functions, consider using borrowed references &stored_static.eventkey_alias to avoid unnecessary cloning. Optimize Rule Processing: Depending on the complexity and size of the rule set, there might be opportunities to optimize the rule processing logic. Consider profiling the rule processing step to identify any bottlenecks or redundant operations. Parallel Processing: If the event records can be processed independently, you can explore parallel processing techniques, such as using rayon's parallel iterators (par_iter) or async/await concurrency, to process the event records concurrently and improve performance. Avoid Unnecessary String Operations: In the _is_target_event_id function, instead of replacing characters in the event ID string (s.replace('"', "")), consider using more efficient methods such as trimming leading and trailing characters or using regular expressions to extract the event ID without unnecessary string manipulations. Use Pattern Matching for Channel Validation: In the _is_valid_channel function, consider using pattern matching on the channel value instead of converting it to a Value and comparing it with a string. This can simplify the logic and potentially improve performance. For example: Optimize Evtx Parsing: If the EvtxParser library supports multi-threaded parsing, consider specifying the number of threads to utilize all available CPU cores effectively. Instead of setting parse_config.num_threads(0), determine the number of available CPU cores dynamically and set parse_config.num_threads(num_cores). Handle Error Conditions: Currently, there are incomplete error handling sections in the code. Make sure to handle potential errors properly, such as when EvtxParser::from_path fails or encounters an error. Handle these cases by displaying an appropriate error message or propagating the error further up the call stack. Output Logo: In the output_logo function, instead of reading the logo content from a file using fs::read_to_string, consider embedding the logo as a string directly in the code. This avoids file I/O and improves performance. You can use a multiline string literal (r#"" ... "#) to store the logo content. Output Easter Egg Arts: The output_eggs function uses a hardcoded mapping of dates to Easter egg arts. Consider abstracting this mapping into a configuration file or data structure to make it easier to add or modify Easter egg arts. This allows for more flexibility without modifying the code. Architecture and Binary Matching: In the is_matched_architecture_and_binary function, ensure that the architecture check is accurate and covers all target platforms. For example, you may need to handle cases where the PROCESSOR_ARCHITEW6432 environment variable is present on 64-bit Windows. Additionally, consider using the std::env::consts::ARCH constant to retrieve the current architecture instead of relying on environment variables. Regarding the output_logo and output_eggs functions, here's an updated version of the code with the improvements mentioned earlier: Make sure to replace 'Your logo content goes here' with the actual logo content you want to display. These updates improve the code by avoiding unnecessary file I/O operations and using a more flexible approach for storing and retrieving Easter egg arts. How to reduce clone:
In the get_all_keys function, instead of collecting the keys into a HashSet and then converting it into a Nested, you can directly create the Nested using the extend method: can be replaced with: This avoids the intermediate HashSet and reduces the need for cloning. In the analysis_file function, instead of cloning the evtx_filepath when passing it to the evtx_to_jsons function, you can pass it by reference: This avoids unnecessary cloning of the PathBuf. In the analysis_json_file function, instead of cloning the path when creating the Arc for arc_path, you can pass a reference to the string: This avoids cloning the path and instead creates a reference-counted pointer to the existing string. In the create_rec_infos function, instead of cloning the path when calling to_string() on arc_path, you can pass a reference to the string:
In the get_all_keys function, you can avoid cloning the keys by directly collecting them into the Nested: This avoids the need to collect the keys into an intermediate HashSet and then convert it to Nested. Use &str instead of String: Updated version: Avoid unnecessary string conversions: Updated version: These examples demonstrate specific optimizations to avoid unnecessary cloning and string conversions. Similar optimizations can be applied to other parts of the codebase by analyzing the specific usage patterns and requirements. Avoid unnecessary string cloning and allocations: Updated version: Use &[u8] instead of Vec where possible: Updated version: Avoid unnecessary cloning of rule keys: Updated version: These examples demonstrate various ways to optimize the code by avoiding unnecessary cloning, string conversions, and allocations. By carefully analyzing the code and its requirements, you can identify specific areas where these optimizations can be applied. |
We would like to reduce the number of clone() uses to reduce memory usage as much as possible.
@fukusuket would you like to try to fix these? I can assign you. If it is difficult then we can ask someone else to take a look at it.
The text was updated successfully, but these errors were encountered: